[SPARK-40919][SQL][TESTS] Fix `AnalysisTest#assertAnalysisErrorClass` to avoid wrong check result when `expectedMessageParameters.size <= 4` #38396

LuciferYang · 2022-10-26T11:59:00Z

What changes were proposed in this pull request?

This pr refactor AnalysisTest#assertAnalysisErrorClass method to use e.messageParameters != expectedMessageParameters instead of !e.messageParameters.sameElements(expectedMessageParameters) to avoid wrong check result when expectedMessageParameters.size <= 4

Why are the changes needed?

Avoid wrong check result of AnalysisTest#assertAnalysisErrorClass when expectedMessageParameters.size <= 4.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GitHub Actions
Manual test:

Welcome to Scala 2.12.17 (OpenJDK 64-Bit Server VM, Java 1.8.0_352).
Type in expressions for evaluation. Or try :help.

scala> :paste
// Entering paste mode (ctrl-D to finish)

val messageParameters = Map(
      "exprName" -> "`window_duration`",
      "valueRange" -> s"(0, 9223372036854775807]",
      "currentValue" -> "-1000000L",
      "sqlExpr" -> "\"window(2016-01-01 01:01:01, -1000000, 1000000, 0)\""
    )
val expectedMessageParameters =  Map(
      "sqlExpr" -> "\"window(2016-01-01 01:01:01, -1000000, 1000000, 0)\"",
      "exprName" -> "`window_duration`",
      "valueRange" -> s"(0, 9223372036854775807]",
      "currentValue" -> "-1000000L"
    )

val tret = !messageParameters.sameElements(expectedMessageParameters)
val fret = messageParameters != expectedMessageParameters


// Exiting paste mode, now interpreting.

messageParameters: scala.collection.immutable.Map[String,String] = Map(exprName -> `window_duration`, valueRange -> (0, 9223372036854775807], currentValue -> -1000000L, sqlExpr -> "window(2016-01-01 01:01:01, -1000000, 1000000, 0)")
expectedMessageParameters: scala.collection.immutable.Map[String,String] = Map(sqlExpr -> "window(2016-01-01 01:01:01, -1000000, 1000000, 0)", exprName -> `window_duration`, valueRange -> (0, 9223372036854775807], currentValue -> -1000000L)
tret: Boolean = true
fret: Boolean = false

LuciferYang · 2022-10-26T12:07:28Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala

      line: Int = -1,
      pos: Int = -1): Unit = {
+
+    def sameMapContents(left: Map[String, String], right: Map[String, String]): Boolean =


cc @MaxGekk fix the problem I mentioned in #38394 (comment)

MaxGekk · 2022-10-26T15:13:05Z

@LuciferYang How about to use ==?

scala> :paste
// Entering paste mode (ctrl-D to finish)

val messageParameters = Map(
      "exprName" -> "`window_duration`",
      "valueRange" -> s"(0, 9223372036854775807]",
      "currentValue" -> "-1000000L",
      "sqlExpr" -> "\"window(2016-01-01 01:01:01, -1000000, 1000000, 0)\""
    )
val expectedMessageParameters =  Map(
      "sqlExpr" -> "\"window(2016-01-01 01:01:01, -1000000, 1000000, 0)\"",
      "exprName" -> "`window_duration`",
      "valueRange" -> s"(0, 9223372036854775807]",
      "currentValue" -> "-1000000L"
    )

// Exiting paste mode, now interpreting.

messageParameters: scala.collection.immutable.Map[String,String] = Map(exprName -> `window_duration`, valueRange -> (0, 9223372036854775807], currentValue -> -1000000L, sqlExpr -> "window(2016-01-01 01:01:01, -1000000, 1000000, 0)")
expectedMessageParameters: scala.collection.immutable.Map[String,String] = Map(sqlExpr -> "window(2016-01-01 01:01:01, -1000000, 1000000, 0)", exprName -> `window_duration`, valueRange -> (0, 9223372036854775807], currentValue -> -1000000L)

scala> messageParameters == expectedMessageParameters
res0: Boolean = true

LuciferYang · 2022-10-26T15:24:54Z

I think == is fine

MaxGekk · 2022-10-26T15:30:54Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala


      if (e.getErrorClass != expectedErrorClass ||
-        !e.messageParameters.sameElements(expectedMessageParameters) ||
+        !(e.messageParameters == expectedMessageParameters) ||


Let's simplify this and use != instead of combination of ! and ==.

MaxGekk

Waiting for CI.

dongjoon-hyun · 2022-10-26T17:21:54Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala


      if (e.getErrorClass != expectedErrorClass ||
-        !e.messageParameters.sameElements(expectedMessageParameters) ||
+        e.messageParameters != expectedMessageParameters ||


nit, Could you fix the indentation by adding two more space here while we are touching this line?

dongjoon-hyun

+1, LGTM (except a minor indentation comment)

LuciferYang · 2022-10-27T02:20:50Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisTest.scala

-        !e.messageParameters.sameElements(expectedMessageParameters) ||
-        (line >= 0 && e.line.getOrElse(-1) != line) ||
-        (pos >= 0) && e.startPosition.getOrElse(-1) != pos) {
+          e.messageParameters != expectedMessageParameters ||


Is the indentation correct now?

MaxGekk · 2022-10-27T05:00:27Z

+1, LGTM. All GAs passed. Merging to master.
Thank you, @LuciferYang and @dongjoon-hyun @HyukjinKwon for review.

LuciferYang · 2022-10-27T05:01:56Z

Thanks @MaxGekk @dongjoon-hyun @HyukjinKwon

… to avoid wrong check result when `expectedMessageParameters.size <= 4` ### What changes were proposed in this pull request? This pr refactor `AnalysisTest#assertAnalysisErrorClass` method to use `e.messageParameters != expectedMessageParameters` instead of `!e.messageParameters.sameElements(expectedMessageParameters)` to avoid wrong check result when `expectedMessageParameters.size <= 4` ### Why are the changes needed? Avoid wrong check result of `AnalysisTest#assertAnalysisErrorClass` when `expectedMessageParameters.size <= 4`. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? - Pass GitHub Actions - Manual test: ```scala Welcome to Scala 2.12.17 (OpenJDK 64-Bit Server VM, Java 1.8.0_352). Type in expressions for evaluation. Or try :help. scala> :paste // Entering paste mode (ctrl-D to finish) val messageParameters = Map( "exprName" -> "`window_duration`", "valueRange" -> s"(0, 9223372036854775807]", "currentValue" -> "-1000000L", "sqlExpr" -> "\"window(2016-01-01 01:01:01, -1000000, 1000000, 0)\"" ) val expectedMessageParameters = Map( "sqlExpr" -> "\"window(2016-01-01 01:01:01, -1000000, 1000000, 0)\"", "exprName" -> "`window_duration`", "valueRange" -> s"(0, 9223372036854775807]", "currentValue" -> "-1000000L" ) val tret = !messageParameters.sameElements(expectedMessageParameters) val fret = messageParameters != expectedMessageParameters // Exiting paste mode, now interpreting. messageParameters: scala.collection.immutable.Map[String,String] = Map(exprName -> `window_duration`, valueRange -> (0, 9223372036854775807], currentValue -> -1000000L, sqlExpr -> "window(2016-01-01 01:01:01, -1000000, 1000000, 0)") expectedMessageParameters: scala.collection.immutable.Map[String,String] = Map(sqlExpr -> "window(2016-01-01 01:01:01, -1000000, 1000000, 0)", exprName -> `window_duration`, valueRange -> (0, 9223372036854775807], currentValue -> -1000000L) tret: Boolean = true fret: Boolean = false ``` Closes apache#38396 from LuciferYang/SPARK-40919. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

fix

733375a

github-actions bot added the SQL label Oct 26, 2022

LuciferYang mentioned this pull request Oct 26, 2022

[SPARK-40759][SQL] Migrate type check failures of time window onto error classes #38394

Closed

LuciferYang commented Oct 26, 2022

View reviewed changes

change to use ==

70eb638

MaxGekk reviewed Oct 26, 2022

View reviewed changes

change to use !=

3d04675

MaxGekk approved these changes Oct 26, 2022

View reviewed changes

dongjoon-hyun reviewed Oct 26, 2022

View reviewed changes

dongjoon-hyun approved these changes Oct 26, 2022

View reviewed changes

HyukjinKwon approved these changes Oct 27, 2022

View reviewed changes

fix indentation

22b76c1

LuciferYang commented Oct 27, 2022

View reviewed changes

MaxGekk closed this in a40f8c1 Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-40919][SQL][TESTS] Fix `AnalysisTest#assertAnalysisErrorClass` to avoid wrong check result when `expectedMessageParameters.size <= 4` #38396

[SPARK-40919][SQL][TESTS] Fix `AnalysisTest#assertAnalysisErrorClass` to avoid wrong check result when `expectedMessageParameters.size <= 4` #38396

Uh oh!

LuciferYang commented Oct 26, 2022 •

edited

Loading

Uh oh!

LuciferYang Oct 26, 2022

Uh oh!

MaxGekk commented Oct 26, 2022

Uh oh!

LuciferYang commented Oct 26, 2022

Uh oh!

MaxGekk Oct 26, 2022

Uh oh!

LuciferYang Oct 26, 2022

Uh oh!

MaxGekk left a comment

Uh oh!

dongjoon-hyun Oct 26, 2022

Uh oh!

dongjoon-hyun left a comment

Uh oh!

LuciferYang Oct 27, 2022 •

edited

Loading

Uh oh!

MaxGekk commented Oct 27, 2022

Uh oh!

LuciferYang commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-40919][SQL][TESTS] Fix AnalysisTest#assertAnalysisErrorClass to avoid wrong check result when expectedMessageParameters.size <= 4 #38396

[SPARK-40919][SQL][TESTS] Fix AnalysisTest#assertAnalysisErrorClass to avoid wrong check result when expectedMessageParameters.size <= 4 #38396

Uh oh!

Conversation

LuciferYang commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

LuciferYang Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Oct 26, 2022

Uh oh!

LuciferYang commented Oct 26, 2022

Uh oh!

MaxGekk Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

LuciferYang Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Oct 27, 2022

Uh oh!

LuciferYang commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[SPARK-40919][SQL][TESTS] Fix `AnalysisTest#assertAnalysisErrorClass` to avoid wrong check result when `expectedMessageParameters.size <= 4` #38396

[SPARK-40919][SQL][TESTS] Fix `AnalysisTest#assertAnalysisErrorClass` to avoid wrong check result when `expectedMessageParameters.size <= 4` #38396

LuciferYang commented Oct 26, 2022 •

edited

Loading

LuciferYang Oct 27, 2022 •

edited

Loading