[SPARK-29359][SQL][TESTS] Better exception handling in (SQL|ThriftServer)QueryTestSuite #26028

peter-toth · 2019-10-04T21:01:49Z

What changes were proposed in this pull request?

This PR adds 2 changes regarding exception handling in SQLQueryTestSuite and ThriftServerQueryTestSuite

fixes an expected output sorting issue in ThriftServerQueryTestSuite as if there is an exception then there is no need for sort
introduces common exception handling in those 2 suites with a new handleExceptions method

Why are the changes needed?

Currently ThriftServerQueryTestSuite passes on master, but it fails on one of my PRs (#23531) with this error (https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/111651/testReport/org.apache.spark.sql.hive.thriftserver/ThriftServerQueryTestSuite/sql_3/):

org.scalatest.exceptions.TestFailedException: Expected "
[Recursion level limit 100 reached but query has not exhausted, try increasing spark.sql.cte.recursion.level.limit
org.apache.spark.SparkException]
", but got "
[org.apache.spark.SparkException
Recursion level limit 100 reached but query has not exhausted, try increasing spark.sql.cte.recursion.level.limit]
" Result did not match for query #4 WITH RECURSIVE r(level) AS (   VALUES (0)   UNION ALL   SELECT level + 1 FROM r ) SELECT * FROM r

The unexpected reversed order of expected output (error message comes first, then the exception class) is due to this line: https://github.com/apache/spark/pull/26028/files#diff-b3ea3021602a88056e52bf83d8782de8L146. It should not sort the expected output if there was an error during execution.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UTs.

peter-toth · 2019-10-04T21:04:21Z

Besides I think this change is useful, I run into some UT failures while testing #23531 which can be fixed by this PR.

peter-toth · 2019-10-04T21:05:27Z

cc @wangyum

dongjoon-hyun · 2019-10-04T22:26:43Z

sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out

 select 30 day day
 -- !query 22 schema
-struct<>
+


Sorry, but let's keep the original form. Most of the changes are due to this, but the first contribution seems to be on the edge.

Thanks @dongjoon-hyun for the review, let me try to convince you that these changes make sense, but if you still disagree just let me know and I will drop them.

The only cases where I replaced the expected schema from struct<> to nothing are those where some error occurs and an exception is thrown. In those cases there is no data returned, so there is no schema at all, not even an empty struct.

-- !query 22 select 30 day day -- !query 22 schema -- !query 22 output org.apache.spark.sql.catalyst.parser.ParseException

IMHO struct<> makes sense where a statement was successful but data returned is empty and there are no columns in it. In those cases I left the expected output intact.

-- !query 0 CREATE OR REPLACE TEMPORARY VIEW view1 AS SELECT 2 AS i1 -- !query 0 schema struct<> -- !query 0 output

Empty expected schema is also useful to easily recognize a statement that ended up in an error (otherwise we probably need to check the output for containing exception which seems less elegant, or the schema containing struct<> and output being non-empty which seems less intuitive).
I utilized empty expected schema to fix a sorting issue in ThriftServerQueryTestSuite: https://github.com/apache/spark/pull/26028/files#diff-b3ea3021602a88056e52bf83d8782de8R147, and there might be other cases in the future where this change could help.

I fully understood that, but it's not worth of this huge change.

All right, I've dropped that part of changes.

SparkQA · 2019-10-05T00:37:54Z

Test build #111787 has finished for PR 26028 at commit 2e1c235.

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

dongjoon-hyun

I'll review this again after the removal of the first one. Thanks.

…ftServerQueryTestSuite

peter-toth · 2019-10-06T17:49:18Z

I'll review this again after the removal of the first one. Thanks.

Ok, thanks. I removed the first one.

dongjoon-hyun · 2019-10-06T18:25:07Z

Thank you for updating, @peter-toth !

dongjoon-hyun · 2019-10-06T18:29:44Z

@peter-toth . For the rest of the contribution, they are a kind of preventive approach and there is no change in the current generated result, right?

Why are the changes needed?

For more robust exception handling.

dongjoon-hyun

Could you give us more concrete example when this PR becomes more meaningful? For now, this seems to be not required urgently.

peter-toth · 2019-10-06T19:37:17Z

Could you give us more concrete example when this PR becomes more meaningful? For now, this seems to be not required urgently.

Currently ThriftServerQueryTestSuite passes on master, but it fails on one of my PRs (#23531) with this error (https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/111651/testReport/org.apache.spark.sql.hive.thriftserver/ThriftServerQueryTestSuite/sql_3/):

org.scalatest.exceptions.TestFailedException: Expected "
[Recursion level limit 100 reached but query has not exhausted, try increasing spark.sql.cte.recursion.level.limit
org.apache.spark.SparkException]
", but got "
[org.apache.spark.SparkException
Recursion level limit 100 reached but query has not exhausted, try increasing spark.sql.cte.recursion.level.limit]
" Result did not match for query #4 WITH RECURSIVE r(level) AS (   VALUES (0)   UNION ALL   SELECT level + 1 FROM r ) SELECT * FROM r

The unexpected reversed order of expected output (error message comes first, then the exception class) is due to this line: https://github.com/apache/spark/pull/26028/files#diff-b3ea3021602a88056e52bf83d8782de8L146. It should not sort the expected output if there was an error during execution.

Other changes belong to the second point, a minor improvement to handle exceptions at a common place. There is no change in generated expected result.

SparkQA · 2019-10-06T21:23:21Z

Test build #111821 has finished for PR 26028 at commit 58e1cf1.

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

dongjoon-hyun · 2019-10-07T16:02:45Z

Got it. Thanks, @peter-toth ! I updated the second section of the PR description with your example.

dongjoon-hyun · 2019-10-07T16:22:36Z

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

-      case _ => plan.children.iterator.exists(isSorted)
-    }
-
+  protected def handleExceptions(result: => (String, Seq[String])): (String, Seq[String]) = {


Could you add a function description because we override this differently?

SQLQueryTestSuite seems to return (struct<>, ...)

ThriftServerQueryTestSuite seems to return ("", answer.sorted)

No, both returns a (String, Seq[String]) tuple where the first is the schema and the second is the result. Since it's impossible to get the exact spark schema back from a java.sql.ResultSet we use empty string in ThriftServerQueryTestSuite.

I've added a description to it and to its override.

dongjoon-hyun · 2019-10-07T16:23:36Z

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

        // with a generic pattern "###".
        val msg = if (a.plan.nonEmpty) a.getSimpleMessage else a.getMessage
-        (StructType(Seq.empty), Seq(a.getClass.getName, msg.replaceAll("#\\d+", "#x")))
+        (emptySchema, Seq(a.getClass.getName, msg.replaceAll("#\\d+", "#x")))


Could you add a test case which this is required?

No particular test case. Since I touched this method and StructType(Seq.empty) was used 3 times so I just moved it to a val.

dongjoon-hyun · 2019-10-07T16:28:09Z

Also, cc @wangyum .

SparkQA · 2019-10-07T22:29:02Z

Test build #111852 has finished for PR 26028 at commit 019e37f.

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

peter-toth · 2019-10-10T06:51:23Z

@dongjoon-hyun @wangyum do you think this PR is ok now?

wangyum

LGTM

dongjoon-hyun · 2019-10-10T16:31:16Z

Hi, @wangyum . You can merge this after manual testing.
Or, we can wait until our Jenkins is back again.

wangyum · 2019-10-12T05:47:40Z

retest this please

SparkQA · 2019-10-12T07:05:02Z

Test build #111951 has finished for PR 26028 at commit 019e37f.

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

wangyum · 2019-10-12T07:28:40Z

retest this please

SparkQA · 2019-10-12T11:08:52Z

Test build #111958 has finished for PR 26028 at commit 019e37f.

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

wangyum · 2019-10-13T05:19:13Z

Thank you @peter-toth @dongjoon-hyun

wangyum · 2019-10-13T05:19:18Z

Merged to master.

peter-toth · 2019-10-13T08:15:11Z

Thank you @dongjoon-hyun and @wangyum for the review.

dongjoon-hyun added the SQL label Oct 4, 2019

dongjoon-hyun changed the title ~~[SPARK-29359] Better exception handling in SQLQueryTestSuite and ThriftServerQueryTestSuite~~ [SPARK-29359][SQL] Better exception handling in SQLQueryTestSuite and ThriftServerQueryTestSuite Oct 4, 2019

dongjoon-hyun changed the title ~~[SPARK-29359][SQL] Better exception handling in SQLQueryTestSuite and ThriftServerQueryTestSuite~~ [SPARK-29359][SQL][TESTS] Better exception handling in (SQL|ThriftServer)QueryTestSuite Oct 4, 2019

dongjoon-hyun added the TESTS label Oct 4, 2019

dongjoon-hyun reviewed Oct 4, 2019

View reviewed changes

dongjoon-hyun requested changes Oct 5, 2019

View reviewed changes

[SPARK-29359] Better exception handling in SQLQueryTestSuite and Thri…

58e1cf1

…ftServerQueryTestSuite

peter-toth force-pushed the SPARK-29359-better-exception-handling branch from 2e1c235 to 58e1cf1 Compare October 6, 2019 17:46

peter-toth requested a review from dongjoon-hyun October 6, 2019 17:48

dongjoon-hyun reviewed Oct 6, 2019

View reviewed changes

dongjoon-hyun reviewed Oct 7, 2019

View reviewed changes

add some comment

019e37f

wangyum approved these changes Oct 10, 2019

View reviewed changes

LantaoJin mentioned this pull request Oct 12, 2019

[SPARK-29283][SQL] Error message is hidden when query from JDBC, especially enabled adaptive execution #25960

Closed

dongjoon-hyun approved these changes Oct 12, 2019

View reviewed changes

wangyum closed this in 9e12c94 Oct 13, 2019

[SPARK-29359][SQL][TESTS] Better exception handling in (SQL|ThriftServer)QueryTestSuite #26028

[SPARK-29359][SQL][TESTS] Better exception handling in (SQL|ThriftServer)QueryTestSuite #26028

Uh oh!

Conversation

peter-toth commented Oct 4, 2019 • edited by dongjoon-hyun 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!

peter-toth commented Oct 4, 2019

Uh oh!

peter-toth commented Oct 4, 2019

Uh oh!

dongjoon-hyun Oct 4, 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!

peter-toth Oct 5, 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!

dongjoon-hyun Oct 5, 2019

Choose a reason for hiding this comment

Uh oh!

peter-toth Oct 6, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 5, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

peter-toth commented Oct 6, 2019

Uh oh!

dongjoon-hyun commented Oct 6, 2019

Uh oh!

dongjoon-hyun commented Oct 6, 2019

Why are the changes needed?

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

peter-toth commented Oct 6, 2019

Uh oh!

SparkQA commented Oct 6, 2019

Uh oh!

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

Uh oh!

Uh oh!

dongjoon-hyun Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

peter-toth Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

peter-toth Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

peter-toth Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 7, 2019

Uh oh!

SparkQA commented Oct 7, 2019

Uh oh!

peter-toth commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangyum left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 10, 2019

Uh oh!

wangyum commented Oct 12, 2019

Uh oh!

peter-toth commented Oct 4, 2019 •

edited by dongjoon-hyun

Loading

dongjoon-hyun Oct 4, 2019 •

edited

Loading

peter-toth Oct 5, 2019 •

edited

Loading

dongjoon-hyun commented Oct 7, 2019 •

edited

Loading

peter-toth commented Oct 10, 2019 •

edited

Loading