[SPARK-28527][SQL][TEST] Re-run all the tests in SQLQueryTestSuite via Thrift Server #25373

wangyum · 2019-08-07T06:36:56Z

What changes were proposed in this pull request?

This PR build a test framework that directly re-run all the tests in SQLQueryTestSuite via Thrift Server. But it's a little different from SQLQueryTestSuite:

Can not support UDF testing.
Can not support DESC command and SHOW command because SQLQueryTestSuite formatted the output.

When building this framework, found two bug:
SPARK-28624: make_date is inconsistent when reading from table
SPARK-28611: Histogram's height is different

found two features that ThriftServer can not support:
SPARK-28636: ThriftServer can not support decimal type with negative scale
SPARK-28637: ThriftServer can not support interval type

Also, found two inconsistent behavior:
SPARK-28620: Double type returned for float type in Beeline/JDBC
SPARK-28619: The golden result file is different when tested by bin/spark-sql

How was this patch tested?

N/A

SparkQA · 2019-08-07T07:05:05Z

Test build #108749 has finished for PR 25373 at commit 699b4db.

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

wangyum · 2019-08-07T07:08:49Z

retest this please

SparkQA · 2019-08-07T08:37:19Z

Test build #108752 has finished for PR 25373 at commit 699b4db.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-08-07T13:35:08Z

Test build #108760 has finished for PR 25373 at commit 908cdc3.

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

wangyum · 2019-08-07T13:42:16Z

Added 112 tests for Thrift Server: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/108760/testReport/org.apache.spark.sql.hive.thriftserver/

gatorsmile · 2019-08-08T00:16:16Z

Can we add the new test suite using their own forked JVMs? I found it is slow. 15 minutes!

SparkQA · 2019-08-08T16:11:03Z

Test build #108826 has finished for PR 25373 at commit 3f21189.

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

juliuszsompolski

@gatorsmile - I assume that starting a new session/connection for every test is intentional for test isolation? It's likely responsible for a big part of the cost of this suite (and original SQLSuite), but I reckon it's necessary to not have it flaky.

juliuszsompolski · 2019-08-12T09:56:50Z

...erver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala

+
+  private var hiveServer2: HiveThriftServer2 = _
+
+  override def beforeEach(): Unit = {


How long does it take to start thriftserver?
Would it be possible to start it once in beforeAll?

The first time is very slow:

[info] ThriftServerQueryTestSuite: [info] - group-by.sql !!! IGNORED !!! Start ThriftServer time: 5599 [info] - natural-join.sql (8 seconds, 404 milliseconds) Start ThriftServer time: 44 [info] - csv-functions.sql (1 second, 30 milliseconds) Start ThriftServer time: 34 [info] - except.sql (8 seconds, 354 milliseconds) Start ThriftServer time: 38 [info] - string-functions.sql (1 second, 281 milliseconds) Start ThriftServer time: 39 [info] - describe-table-column.sql (2 seconds, 434 milliseconds) Start ThriftServer time: 81 [info] - random.sql (621 milliseconds) Start ThriftServer time: 30 [info] - tablesample-negative.sql (485 milliseconds) Start ThriftServer time: 30 [info] - window.sql (25 seconds, 601 milliseconds) Start ThriftServer time: 32 [info] - join-empty-relation.sql (632 milliseconds) Start ThriftServer time: 35 [info] - null-propagation.sql (310 milliseconds) Start ThriftServer time: 33 [info] - operators.sql (1 second, 683 milliseconds) Start ThriftServer time: 33 [info] - change-column.sql (504 milliseconds) Start ThriftServer time: 33 [info] - count.sql (2 seconds, 125 milliseconds) [info] - decimalArithmeticOperations.sql !!! IGNORED !!! Start ThriftServer time: 31 [info] - group-analytics.sql (16 seconds, 295 milliseconds) Start ThriftServer time: 34 [info] - inline-table.sql (430 milliseconds) Start ThriftServer time: 29 [info] - comparator.sql (240 milliseconds) Start ThriftServer time: 27

It still takes 16 minutes after move startThriftServer from beforeEach to beforeAll.

startThriftServer at beforeAll:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/109036/testReport/org.apache.spark.sql.hive.thriftserver/
startThriftServer at beforeEach:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/108826/testReport/org.apache.spark.sql.hive.thriftserver/

juliuszsompolski · 2019-08-12T10:14:30Z

...erver/src/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerQueryTestSuite.scala

+    hiveServer2 = HiveThriftServer2.startWithContext(sqlContext)
+  }
+
+  private def withJdbcStatement(fs: (Statement => Unit)*) {


Could we keep the connection open, to not have to start a sessopm amd reload the test data into temp views each time?
We could open the connection with conn = DriverManager.getConnection(jdbcUri, user, "") in beforeAll, load the test data there, and then have withJDBCStatement just create new statements, finally closing the connection in afterAll.

However, it seems that opening new connection/session may be by design here, for test isolation. Then we'll have to leave it as is, but I think we should still be able to avoid starting the ThriftServer beforeEach.

If move loadTestData to beforeAll. Some tests will fail:

SparkQA · 2019-08-13T13:28:48Z

Test build #109036 has finished for PR 25373 at commit 5802888.

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

juliuszsompolski

LGTM! Thanks. This really improves coverage and has flushed multiple issues already. Let's get it in :-).
cc @gatorsmile

gatorsmile · 2019-08-18T02:12:48Z

Thanks! Merged to master.

dongjoon-hyun · 2019-08-18T23:30:58Z

Hi, Guys.

This seems to add a new flaky test suite in Maven with hadoop-3.2 profile.

master (Maven and hadoop-3.2): https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-3.2/230/consoleFull

ThriftServerQueryTestSuite:
org.apache.spark.sql.hive.thriftserver.ThriftServerQueryTestSuite *** ABORTED ***
  java.lang.RuntimeException: Unable to load a Suite class that was discovered in the runpath: org.apache.spark.sql.hive.thriftserver.ThriftServerQueryTestSuite

In general, hadoop-3.2 profile is not a blocker, but we are working on a JDK11 PR with Hadoop-3.2. And, it fails due to this. Could you take a look at this?

[SPARK-28723][SQL] Upgrade to Hive 2.3.6 for HiveMetastore Client and Hadoop-3.2 profile #25443 (comment)

cc @wangyum, @srowen , @HyukjinKwon .

I'm monitoring the next Jenkins run. If the next run fails consecutively, we had better revert this first and merge this later after testing with Maven with hadoop-3.2 profile.

dongjoon-hyun · 2019-08-18T23:53:09Z

I found that this also breaks hadoop-2.7 profile.

master (Maven and hadoop-2.7) https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.7/6744/consoleFull

ThriftServerQueryTestSuite:
org.apache.spark.sql.hive.thriftserver.ThriftServerQueryTestSuite *** ABORTED ***
  java.lang.RuntimeException: Unable to load a Suite class that was discovered in the runpath:

Since this breaks all Maven profiles, I'll revert this. Sorry, @wangyum and @gatorsmile . Please make another PR and test with Maven Hadoop-2.7/Hadoop-3.2.

HyukjinKwon · 2019-08-19T00:13:56Z

+1 for reverting for now.

wangyum · 2019-08-19T10:25:40Z

The reason is that the path is different:
maven:

path: /root/opensource/spark/sql/hive-thriftserver/file:/root/opensource/spark/sql/core/target/spark-sql_2.12-3.0.0-SNAPSHOT-tests.jar!/sql-tests/inputs

sbt:

path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/subquery
ppath: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/subquery/negative-cases
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/subquery/exists-subquery
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/subquery/in-subquery
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/subquery/scalar-subquery
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/typeCoercion
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/typeCoercion/native
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/pgSQL
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/ansi
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/udf
path: /root/opensource/spark/sql/core/target/scala-2.12/test-classes/sql-tests/inputs/udf/pgSQL

srowen · 2019-08-19T14:35:39Z

PS there is more to the failure:

  Cause: java.lang.NullPointerException:
  at scala.collection.mutable.ArrayOps$ofRef$.newBuilder$extension(ArrayOps.scala:202)
...
  at scala.collection.mutable.ArrayOps$ofRef.partition(ArrayOps.scala:198)
  at org.apache.spark.sql.SQLQueryTestSuite.listFilesRecursively(SQLQueryTestSuite.scala:453)
  at org.apache.spark.sql.hive.thriftserver.ThriftServerQueryTestSuite.listTestCases(ThriftServerQueryTestSuite.scala:207)
...

It does sorta look like it can't list a directory because it's 'empty'? that is listFiles() is null. The subdirs of sql-tests/ are packaged into that -tests JAR though.

Comments in SQLQueryTestSuite suggest that it's known that SBT will have these resources as local files in the classpath (not in the JAR) -- see the comment on baseResourcePath. So, hm, has SBT always worked differently in this regard?

But then I don't know how Maven builds have ever worked for SQLQueryTestSuite as they should use the exact same mechanism? It appears to be running in the failed build, of course.

Weirder still is that it doesn't happen consistently?

Well yeah I think we'd have to revert for now.

juliuszsompolski · 2019-08-22T12:53:57Z

@dongjoon-hyun @srowen @HyukjinKwon @wangyum
Do we have any idea how to fix it?
I personally never used maven build, always sbt, so I have no idea.

juliuszsompolski · 2019-08-22T13:06:48Z

Just guessing: can it be an issue that for SQLQueryTestSuite, these files were resources in the same project jar, while for ThriftServerQueryTestSuite it needs to pull them from a dependency?

A guess based on https://stackoverflow.com/questions/5292283/use-a-dependencys-resources: maybe changing https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L117
getClass.getClassLoader.getResource("sql-tests") to SQLQueryTestSuite.class.getClassLoader.getResource("sql-tests") will work here?

srowen · 2019-08-22T13:14:22Z

I doubt those two would be in different classloaders, but, I also don't know what the issue is.

wangyum · 2019-08-22T15:29:59Z

Could we skip this test when testing with maven?

   override def listTestCases(): Seq[TestCase] = {
-    listFilesRecursively(new File(inputFilePath)).flatMap { file =>
-      val resultFile = file.getAbsolutePath.replace(inputFilePath, goldenFilePath) + ".out"
-      val absPath = file.getAbsolutePath
-      val testCaseName = absPath.stripPrefix(inputFilePath).stripPrefix(File.separator)
+    // Maven can not get the correct baseResourcePath
+    if (baseResourcePath.exists()) {
+      listFilesRecursively(new File(inputFilePath)).flatMap { file =>
+        val resultFile = file.getAbsolutePath.replace(inputFilePath, goldenFilePath) + ".out"
+        val absPath = file.getAbsolutePath
+        val testCaseName = absPath.stripPrefix(inputFilePath).stripPrefix(File.separator)
 
-      if (file.getAbsolutePath.startsWith(s"$inputFilePath${File.separator}udf")) {
-        Seq.empty
-      } else if (file.getAbsolutePath.startsWith(s"$inputFilePath${File.separator}pgSQL")) {
-        PgSQLTestCase(testCaseName, absPath, resultFile) :: Nil
-      } else {
-        RegularTestCase(testCaseName, absPath, resultFile) :: Nil
+        if (file.getAbsolutePath.startsWith(s"$inputFilePath${File.separator}udf")) {
+          Seq.empty
+        } else if (file.getAbsolutePath.startsWith(s"$inputFilePath${File.separator}pgSQL")) {
+          PgSQLTestCase(testCaseName, absPath, resultFile) :: Nil
+        } else {
+          RegularTestCase(testCaseName, absPath, resultFile) :: Nil
+        }
       }
+    } else {
+      Seq.empty[TestCase]
     }
   }

dongjoon-hyun · 2019-08-22T18:55:53Z

Ur, @wangyum . AFAIK, Apache Maven is the official primary Apache Spark project build system. We support sbt, but sbt is not the main.

Could you confirm this, @srowen ?

srowen · 2019-08-22T19:02:51Z

Yeah, Maven is the 'build of reference' so I'd hesitate to have any SBT-only tests. I think we'd have to debug and fix it, though I expect this one is subtle and I couldn't figure it out in an hour

…a Thrift Server ## What changes were proposed in this pull request? This PR build a test framework that directly re-run all the tests in `SQLQueryTestSuite` via Thrift Server. But it's a little different from `SQLQueryTestSuite`: 1. Can not support [UDF testing](https://github.com/apache/spark/blob/44e607e9213bdceab970606fb15292db2fe157c2/sql/core/src/test/scala/org/apache/spark/sql/SQLQueryTestSuite.scala#L293-L297). 2. Can not support `DESC` command and `SHOW` command because `SQLQueryTestSuite` [formatted the output](https://github.com/apache/spark/blob/1882912cca4921d3d8c8632b3bb34e69e8119791/sql/core/src/main/scala/org/apache/spark/sql/execution/HiveResult.scala#L38-L50.). When building this framework, found two bug: [SPARK-28624](https://issues.apache.org/jira/browse/SPARK-28624): `make_date` is inconsistent when reading from table [SPARK-28611](https://issues.apache.org/jira/browse/SPARK-28611): Histogram's height is different found two features that ThriftServer can not support: [SPARK-28636](https://issues.apache.org/jira/browse/SPARK-28636): ThriftServer can not support decimal type with negative scale [SPARK-28637](https://issues.apache.org/jira/browse/SPARK-28637): ThriftServer can not support interval type Also, found two inconsistent behavior: [SPARK-28620](https://issues.apache.org/jira/browse/SPARK-28620): Double type returned for float type in Beeline/JDBC [SPARK-28619](https://issues.apache.org/jira/browse/SPARK-28619): The golden result file is different when tested by `bin/spark-sql` ## How was this patch tested? N/A Closes apache#25373 from wangyum/SPARK-28527. Authored-by: Yuming Wang <[email protected]> Signed-off-by: gatorsmile <[email protected]>

HyukjinKwon · 2019-08-25T04:45:00Z

Thanks for pinging me @juliuszsompolski. One possible solution might be just always users sql-tests from the source by Spark home which is set by default. Seems similar way is already used when SPARK_GENERATE_GOLDEN_FILES is enabled.

Referring spark.home property or SPARK_HOME is a proper way to detect some paths too. I think it's not crazy to use this way. Let me open a PR

Directly re-run all the tests in SQLQueryTestSuite via Thrift Server

699b4db

dongjoon-hyun added SQL TESTS labels Aug 7, 2019

avoid test with ConfigSets

908cdc3

wangyum added 2 commits August 8, 2019 10:17

Merge remote-tracking branch 'upstream/master' into SPARK-28527

66e9b25

Add ThriftServerQueryTestSuite to RunInTheirOwnDedicatedJvm

3f21189

juliuszsompolski reviewed Aug 12, 2019

View reviewed changes

Move startThriftServer from beforeEach to beforeAll

5802888

wangyum changed the title ~~[SPARK-28527][SQL][TEST] Directly re-run all the tests in SQLQueryTestSuite via Thrift Server~~ [SPARK-28527][SQL][TEST] Re-run all the tests in SQLQueryTestSuite via Thrift Server Aug 13, 2019

juliuszsompolski approved these changes Aug 14, 2019

View reviewed changes

gatorsmile closed this in efbb035 Aug 18, 2019

wangyum deleted the SPARK-28527 branch August 18, 2019 02:42

dongjoon-hyun mentioned this pull request Aug 19, 2019

[SPARK-28723][SQL] Upgrade to Hive 2.3.6 for HiveMetastore Client and Hadoop-3.2 profile #25443

Closed

HyukjinKwon mentioned this pull request Aug 25, 2019

[SPARK-28527][SQL][TEST][test-maven] Re-run all the tests in SQLQueryTestSuite via Thrift Server #25574

Closed


		private var hiveServer2: HiveThriftServer2 = _

		override def beforeEach(): Unit = {

[SPARK-28527][SQL][TEST] Re-run all the tests in SQLQueryTestSuite via Thrift Server #25373

[SPARK-28527][SQL][TEST] Re-run all the tests in SQLQueryTestSuite via Thrift Server #25373

Uh oh!

Conversation

wangyum commented Aug 7, 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?

How was this patch tested?

Uh oh!

SparkQA commented Aug 7, 2019

Uh oh!

wangyum commented Aug 7, 2019

Uh oh!

SparkQA commented Aug 7, 2019

Uh oh!

SparkQA commented Aug 7, 2019

Uh oh!

wangyum commented Aug 7, 2019

Uh oh!

gatorsmile commented Aug 8, 2019

Uh oh!

SparkQA commented Aug 8, 2019

Uh oh!

juliuszsompolski left a comment

Choose a reason for hiding this comment

Uh oh!

juliuszsompolski Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

wangyum Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

wangyum Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

juliuszsompolski Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

wangyum Aug 13, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 13, 2019

Uh oh!

juliuszsompolski left a comment

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 18, 2019

Uh oh!

dongjoon-hyun commented Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Aug 18, 2019

Uh oh!

HyukjinKwon commented Aug 19, 2019

Uh oh!

wangyum commented Aug 19, 2019

Uh oh!

srowen commented Aug 19, 2019

Uh oh!

juliuszsompolski commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juliuszsompolski commented Aug 22, 2019

Uh oh!

srowen commented Aug 22, 2019

Uh oh!

wangyum commented Aug 22, 2019

Uh oh!

dongjoon-hyun commented Aug 22, 2019

Uh oh!

srowen commented Aug 22, 2019

Uh oh!

HyukjinKwon commented Aug 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

wangyum commented Aug 7, 2019 •

edited

Loading

dongjoon-hyun commented Aug 18, 2019 •

edited

Loading

juliuszsompolski commented Aug 22, 2019 •

edited

Loading