[SPARK-30159][SQL][TESTS] Fix the method calls of `QueryTest.checkAnswer` #26788

gengliangwang · 2019-12-06T22:36:10Z

What changes were proposed in this pull request?

Before this PR, the method checkAnswer in Object QueryTest returns an optional string. It doesn't throw exceptions when errors happen.
The actual exceptions are thrown in the trait QueryTest.

However, there are some test suites(StreamSuite, SessionStateSuite, BinaryFileFormatSuite, etc.) that use the no-op method QueryTest.checkAnswer and expect it to fail test cases when the execution results don't match the expected answers.

After this PR:

the method checkAnswer in Object QueryTest will fail tests on errors or unexpected results.
add a new method getErrorMessageInCheckAnswer, which is exactly the same as the previous version of checkAnswer. There are some test suites use this one to customize the test failure message.
for the test suites that extend the trait QueryTest, we should use the method checkAnswer directly, instead of calling the method from Object QueryTest.

Why are the changes needed?

We should fix these method calls to perform actual validations in test suites.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing unit tests.

SparkQA · 2019-12-06T22:54:04Z

Test build #114968 has finished for PR 26788 at commit a73fda4.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-12-06T22:57:10Z

cc @wangyum

SparkQA · 2019-12-06T23:18:23Z

Test build #114969 has finished for PR 26788 at commit 175f3b9.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

gengliangwang · 2019-12-06T23:37:16Z

@wangyum I ran the following command in my local laptop

./build/sbt -Phadoop-2.7 -Phive-2.3 -Pyarn -Phive -Pmesos -Pkinesis-asl -Pspark-ganglia-lgpl -Pkubernetes -Phadoop-cloud -Phive-thriftserver unidoc

and got errors:

[error] /Users/gengliang.wang/Downloads/spark/sql/hive-thriftserver/v2.3/src/main/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java:248:  error: incompatible types: org.apache.hive.service.rpc.thrift.TSessionHandle cannot be converted to org.apache.hive.service.cli.thrift.TSessionHandle
[error]       resp.setSessionHandle(sessionHandle.toTSessionHandle());
[error]                                                           ^
[error] /Users/gengliang.wang/Downloads/spark/sql/hive-thriftserver/v2.3/src/main/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java:259:  error: incompatible types: org.apache.hive.service.rpc.thrift.TStatus cannot be converted to org.apache.hive.service.cli.thrift.TStatus
[error]       resp.setStatus(HiveSQLException.toTStatus(e));
[error]                                                ^
[error] /Users/gengliang.wang/Downloads/spark/sql/hive-thriftserver/v2.3/src/main/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java:346:  error: method getMinVersion in class ThriftCLIService cannot be applied to given types;
[error]     TProtocolVersion protocol = getMinVersion(CLIService.SERVER_VERSION,
[error]

Any suggestions? Thank you in advance!

HyukjinKwon · 2019-12-07T02:05:25Z

Is it a duplicate of #26768?

HyukjinKwon · 2019-12-07T02:06:21Z

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala

+   * @param expectedAnswer the expected result in a [[Seq]] of [[Row]]s.
+   * @param checkToRDD whether to verify deserialization to an RDD. This runs the query twice.
+   */
+  def checkAnswer(df: DataFrame, expectedAnswer: Seq[Row], checkToRDD: Boolean = true): Unit = {


We can make it private if this isn't supposed to be called in the tests directly. companion class still can access.

There are some suites needs this one: ReduceNumShufflePartitionsSuite, SessionStateSuite and SQLQuerySuite(SQLQuerySuite needs to set the parameter checkToRDD as false)
And I prefer not to have duplicated code in these 3 suites.

HeartSaVioR · 2019-12-07T02:12:16Z

Looks like dup. of #26768

gengliangwang · 2019-12-07T04:17:37Z

@wangyum @dongjoon-hyun @HyukjinKwon @HeartSaVioR
I am really sorry I didn't know this is duplicated with #26768 (I even though Doogjoon pinged Yuming for the test failure...)
However, I take a look at #26768 now and I think the code changes in this one are simpler. I also fix some usages which should call the method checkAnswer in the trait QueryTest

gengliangwang · 2019-12-07T04:28:48Z

retest this please.

SparkQA · 2019-12-07T04:45:10Z

Test build #114972 has finished for PR 26788 at commit 175f3b9.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2019-12-07T07:04:15Z

Thank you @gengliangwang . I'm close #26768

wangyum · 2019-12-07T07:05:13Z

@wangyum I ran the following command in my local laptop

./build/sbt -Phadoop-2.7 -Phive-2.3 -Pyarn -Phive -Pmesos -Pkinesis-asl -Pspark-ganglia-lgpl -Pkubernetes -Phadoop-cloud -Phive-thriftserver unidoc

and got errors:

[error] /Users/gengliang.wang/Downloads/spark/sql/hive-thriftserver/v2.3/src/main/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java:248:  error: incompatible types: org.apache.hive.service.rpc.thrift.TSessionHandle cannot be converted to org.apache.hive.service.cli.thrift.TSessionHandle
[error]       resp.setSessionHandle(sessionHandle.toTSessionHandle());
[error]                                                           ^
[error] /Users/gengliang.wang/Downloads/spark/sql/hive-thriftserver/v2.3/src/main/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java:259:  error: incompatible types: org.apache.hive.service.rpc.thrift.TStatus cannot be converted to org.apache.hive.service.cli.thrift.TStatus
[error]       resp.setStatus(HiveSQLException.toTStatus(e));
[error]                                                ^
[error] /Users/gengliang.wang/Downloads/spark/sql/hive-thriftserver/v2.3/src/main/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java:346:  error: method getMinVersion in class ThriftCLIService cannot be applied to given types;
[error]     TProtocolVersion protocol = getMinVersion(CLIService.SERVER_VERSION,
[error]

Any suggestions? Thank you in advance!

Cloud you try:

./build/sbt clean -Phadoop-2.7 -Phive-2.3 -Pyarn -Phive -Pmesos -Pkinesis-asl -Pspark-ganglia-lgpl -Pkubernetes -Phadoop-cloud -Phive-thriftserver unidoc

wangyum · 2019-12-07T07:06:48Z

sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamSuite.scala

-        QueryTest.checkAnswer(spark.table("counts").toDF(),
+        checkAnswer(spark.table("counts").toDF(),
          Row("1", 1) :: Row("2", 1) :: Row("3", 2) :: Row("4", 2) ::
          Row("5", 2) :: Row("6", 2) :: Row("7", 1) :: Row("8", 1) :: Row("9", 1) :: Nil)


This should fail because the types do not match: https://github.com/apache/spark/pull/26768/files#r354499485

Got it, thanks!

gengliangwang · 2019-12-07T10:06:05Z

dev/run-tests.py

    # Enable all of the profiles for the build:
    build_profiles = extra_profiles + modules.root.build_profile_flags
-    sbt_goals = ["unidoc"]
+    sbt_goals = ["clean", "unidoc"]


This is not relevant to this PR. I am trying if the jenkins tests are passed with it.

Nice. It passed now. Could you make a new PR for this line first?

SparkQA · 2019-12-07T12:31:38Z

Test build #114978 has finished for PR 26788 at commit f824f26.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gengliangwang · 2019-12-07T22:48:12Z

mark this one as WIP before #26796 is merged.

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala

sql/core/src/test/scala/org/apache/spark/sql/execution/ReduceNumShufflePartitionsSuite.scala

HeartSaVioR · 2019-12-08T01:47:28Z

...test/scala/org/apache/spark/sql/execution/datasources/binaryfile/BinaryFileFormatSuite.scala

      }
      val expected = Seq(Row(content))
-      QueryTest.checkAnswer(readContent(), expected)
+      checkAnswer(readContent, expected)


nit: maybe either removing () from function or roll back the change of removing ()? It's not just retrieving the value, adding () seems to be semantically correct.

SparkQA · 2019-12-08T05:21:38Z

Test build #114988 has finished for PR 26788 at commit 02c9935.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-08T08:05:01Z

Test build #114989 has finished for PR 26788 at commit 25fcbb8.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR · 2019-12-08T08:26:39Z

Retest this, please

SparkQA · 2019-12-08T11:04:00Z

Test build #114990 has finished for PR 26788 at commit 25fcbb8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HeartSaVioR

LGTM

srowen · 2019-12-08T15:00:46Z

sql/core/src/test/java/test/org/apache/spark/sql/JavaSaveLoadSuite.java

  private static void checkAnswer(Dataset<Row> actual, List<Row> expected) {
-    String errorMessage = QueryTest$.MODULE$.checkAnswer(actual, expected);
+    String errorMessage = QueryTest$.MODULE$.getErrorMessageInCheckAnswer(actual, expected);
    if (errorMessage != null) {


This construction seems kinda pointless then. Why not just let checkAnswer fail with an assertion, rather than have a separate method that only returns the error and have tests manually check if there's an error string and fail it?

If there's no issue on calling fail from scalatest in java suite (mixed-up usage), that seems the way to go; otherwise we may be able to have another QueryTest as utility class for java suite, and move this method instead and reuse across java suites.

I think it's OK; it will generate a relevant failure exception and message. I'd prefer it for simplicity.

SparkQA · 2019-12-09T01:14:04Z

Test build #114999 has finished for PR 26788 at commit bfbace1.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-09T01:49:20Z

Test build #115001 has finished for PR 26788 at commit 6e70776.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-12-09T03:15:44Z

@gengliangwang, can you try to pick 8c06512 and see if it works here? I checked in local but wanted to be very sure it fixes.

SparkQA · 2019-12-09T04:01:32Z

Test build #115000 has finished for PR 26788 at commit 970ec48.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-12-09T04:06:59Z

Test build #115010 has finished for PR 26788 at commit 1384694.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala

cloud-fan · 2019-12-09T05:17:15Z

LGTM

SparkQA · 2019-12-09T08:05:02Z

Test build #115011 has finished for PR 26788 at commit 7fd4057.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

gengliangwang · 2019-12-09T08:14:02Z

retest this please.

SparkQA · 2019-12-09T12:05:54Z

Test build #115018 has finished for PR 26788 at commit 7fd4057.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2019-12-09T13:19:02Z

Merged to master.

…mports ### What changes were proposed in this pull request? This patch fixes the Java code style violations in SPARK-30159 (#26788) which are caught by lint-java (Github Action caught it and I can reproduce it locally). Looks like Jenkins build may have different policy on checking Java style check or less accurate. ### Why are the changes needed? Java linter starts complaining. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? lint-java passed locally This closes #26819 Closes #26818 from HeartSaVioR/SPARK-30159-FOLLOWUP. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

dongjoon-hyun · 2019-12-09T16:59:05Z

@HeartSaVioR fixed the lint-java error with the followup (#26818).

dongjoon-hyun added SQL TESTS labels Dec 6, 2019

HyukjinKwon reviewed Dec 7, 2019

View reviewed changes

wangyum reviewed Dec 7, 2019

View reviewed changes

gengliangwang commented Dec 7, 2019

View reviewed changes

gengliangwang mentioned this pull request Dec 7, 2019

[SPARK-30164][Tests][Doc] SBT clean before document generation #26796

Closed

gengliangwang changed the title ~~[SPARK-30159][SQL][TESTS] Fix the method calls of QueryTest.checkAnswer~~ [WIP][SPARK-30159][SQL][TESTS] Fix the method calls of QueryTest.checkAnswer Dec 7, 2019

HeartSaVioR reviewed Dec 8, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala Outdated Show resolved Hide resolved

HeartSaVioR reviewed Dec 8, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/ReduceNumShufflePartitionsSuite.scala Outdated Show resolved Hide resolved

HeartSaVioR reviewed Dec 8, 2019

View reviewed changes

HeartSaVioR approved these changes Dec 8, 2019

View reviewed changes

srowen reviewed Dec 8, 2019

View reviewed changes

HyukjinKwon mentioned this pull request Dec 9, 2019

[SPARK-30164][TESTS][DOCS] Exclude Hive domain in Unidoc build explicitly #26800

Closed

fix method calls of checkAnswer

9d92726

gengliangwang force-pushed the fixCheckAnswer branch from 6e70776 to 1384694 Compare December 9, 2019 03:52

HyukjinKwon reviewed Dec 9, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Dec 9, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/QueryTest.scala Outdated Show resolved Hide resolved

remove [[]] in code comment

7fd4057

gengliangwang force-pushed the fixCheckAnswer branch from 1384694 to 7fd4057 Compare December 9, 2019 04:49

HyukjinKwon approved these changes Dec 9, 2019

View reviewed changes

gengliangwang changed the title ~~[WIP][SPARK-30159][SQL][TESTS] Fix the method calls of QueryTest.checkAnswer~~ [SPARK-30159][SQL][TESTS] Fix the method calls of QueryTest.checkAnswer Dec 9, 2019

HyukjinKwon closed this in a717d21 Dec 9, 2019

HeartSaVioR mentioned this pull request Dec 9, 2019

[SPARK-30159][SQL][FOLLOWUP] Fix lint-java via removing unnecessary imports #26818

Closed

dongjoon-hyun mentioned this pull request Dec 11, 2019

[SPARK-27189][CORE] Add Executor metrics and memory usage instrumentation to the metrics system #24132

Closed

[SPARK-30159][SQL][TESTS] Fix the method calls of QueryTest.checkAnswer #26788

[SPARK-30159][SQL][TESTS] Fix the method calls of QueryTest.checkAnswer #26788

Uh oh!

Conversation

gengliangwang commented Dec 6, 2019 • 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!

SparkQA commented Dec 6, 2019

Uh oh!

dongjoon-hyun commented Dec 6, 2019

Uh oh!

SparkQA commented Dec 6, 2019

Uh oh!

gengliangwang commented Dec 6, 2019

Uh oh!

HyukjinKwon commented Dec 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented Dec 7, 2019

Uh oh!

gengliangwang commented Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gengliangwang commented Dec 7, 2019

Uh oh!

SparkQA commented Dec 7, 2019

Uh oh!

wangyum commented Dec 7, 2019

Uh oh!

wangyum commented Dec 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 7, 2019

Uh oh!

gengliangwang commented Dec 7, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 8, 2019

Uh oh!

SparkQA commented Dec 8, 2019

Uh oh!

HeartSaVioR commented Dec 8, 2019

Uh oh!

SparkQA commented Dec 8, 2019

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Dec 8, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 9, 2019

Uh oh!

SparkQA commented Dec 9, 2019

Uh oh!

HyukjinKwon commented Dec 9, 2019

[SPARK-30159][SQL][TESTS] Fix the method calls of `QueryTest.checkAnswer` #26788

[SPARK-30159][SQL][TESTS] Fix the method calls of `QueryTest.checkAnswer` #26788

gengliangwang commented Dec 6, 2019 •

edited

Loading

gengliangwang commented Dec 7, 2019 •

edited

Loading

HeartSaVioR Dec 8, 2019 •

edited

Loading