[SPARK-22774][SQL][Test] Add compilation check into TPCDSQuerySuite #19971

kiszk · 2017-12-13T17:11:46Z

What changes were proposed in this pull request?

This PR adds check whether Java code generated by Catalyst can be compiled by janino correctly or not into TPCDSQuerySuite. Before this PR, this suite only checks whether analysis can be performed correctly or not.

This check will be able to avoid unexpected performance degrade by interpreter execution due to a Java compilation error.

How was this patch tested?

Existing a test case, but updated it.

mgaido91 · 2017-12-13T17:22:08Z

wrong JIRA number, may you update it please? thanks.

kiszk · 2017-12-13T17:22:50Z

Thanks, done

SparkQA · 2017-12-13T19:19:57Z

Test build #84872 has finished for PR 19971 at commit 19a0d2d.

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

gatorsmile · 2017-12-13T19:55:25Z

: )

maropu · 2017-12-13T22:56:46Z

sql/core/src/main/scala/org/apache/spark/sql/execution/debug/package.scala

we essentially need the changes in this file? I feel it's okay to touch TPCDSQuerySuite.scala only like: master...maropu:SPARK-22140-FOLLOWUP

Thanks, we can do. I wanted to reduce duplicated code.

probably, the debug package is not for test uses like this.

maropu · 2017-12-13T23:58:21Z

retest this please

JoshRosen · 2017-12-14T01:10:09Z

Could we address this more broadly by ensuring that whole stage codegen fallback is disabled in tests?

JoshRosen · 2017-12-14T01:13:03Z

Wait, nevermind: I see that this isn't actually running the queries, so setting spark.sql.codegen.fallback=false wouldn't be sufficient.

Separate from this PR, we might want to set spark.sql.codegen.fallback=false in more tests, though: it looks like it's on for Hive tests but might be disabled in some SQL suites.

cloud-fan · 2017-12-14T01:19:21Z

We don't need to as we never fallback in test

// try to compile and fallback if it failed
    val (_, maxCodeSize) = try {
      CodeGenerator.compile(cleanedSource)
    } catch {
      case _: Exception if !Utils.isTesting && sqlContext.conf.codegenFallback =>
        // We should already saw the error message
        logWarning(s"Whole-stage codegen disabled for this plan:\n $treeString")
        return child.execute()
    }

viirya · 2017-12-14T02:20:53Z

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

Is it helpful to show up the subtree which causes the compilation error?

SparkQA · 2017-12-14T02:45:25Z

Test build #84886 has finished for PR 19971 at commit 19a0d2d.

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

cloud-fan · 2017-12-14T06:06:26Z

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

+        codegenSubtrees += s
+      case s => s
+    }
+    codegenSubtrees.toSeq.map { subtree =>


nit: map -> foreach

cloud-fan · 2017-12-14T06:08:20Z

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

+          val msg =
+            s"Subtree:\n$subtree\n" +
+            s"Generated code:\n${CodeFormatter.format(code)}\n"
+          logDebug(msg)


it's more useful to include the error message in the exception, e.g. throw new Exception(msg, e)

SparkQA · 2017-12-14T08:05:01Z

Test build #84894 has finished for PR 19971 at commit 1480ae3.

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

maropu · 2017-12-14T10:47:22Z

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

+        CodeGenerator.compile(code)
+      } catch {
+        case e: Exception =>
+          logError(s"failed to compile: $e", e)


We need this log? I feel a bit meaningless.

I have no strong preference. I followed the following code in CodeGenerator.scala

val maxCodeSize = try { evaluator.cook("generated.java", code.body) updateAndGetCompilationStats(evaluator) } catch { case e: InternalCompilerException => val msg = s"failed to compile: $e" logError(msg, e) val maxLines = SQLConf.get.loggingMaxLinesForCodegen logInfo(s"\n${CodeFormatter.format(code, maxLines)}") throw new InternalCompilerException(msg, e) case e: CompileException => val msg = s"failed to compile: $e" logError(msg, e) val maxLines = SQLConf.get.loggingMaxLinesForCodegen logInfo(s"\n${CodeFormatter.format(code, maxLines)}") throw new CompileException(msg, e.getLocation) }

ya, I know though, the logging is there because compilation errors lead to the fallbacks and then the exception messages are not passed to users. But, in this test case, we explicitly throw an exception below with the message. So, I said a bit meaningless.

CodeGenerator is production code, we usually don't log in tests

we can combine it with the exception error message

val msg = s""" |failed to compile: |Subtree: ... """

SparkQA · 2017-12-14T12:25:25Z

Test build #84906 has finished for PR 19971 at commit 7d2c599.

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

maropu · 2017-12-14T13:39:56Z

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

 * without hitting the max iteration threshold.
 */
-class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfterAll {
+class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfterAll with Logging {


nit: drop Logging and the import

cloud-fan · 2017-12-14T15:06:35Z

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

+               |$subtree
+               |Generated code:
+               |${CodeFormatter.format(code)}
+             """


stripMargin

Actually this should be generated by IDE automatically...

Good catch, while I am using IntelliJ...

cloud-fan · 2017-12-14T15:26:30Z

LGTM

SparkQA · 2017-12-14T16:16:31Z

Test build #84913 has finished for PR 19971 at commit dacb790.

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

SparkQA · 2017-12-14T18:09:25Z

Test build #84917 has finished for PR 19971 at commit 44abd2f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfterAll

cloud-fan · 2017-12-14T18:14:56Z

thanks, merging to master!

gatorsmile · 2017-12-14T19:57:23Z

For safety, I also add a TPC-H query suite to do the same checks like TPC-DS, #19982

kiszk changed the title ~~[SPARK-22773][SQL][Test] Add compilation check into TPCDSQuerySuite~~ [SPARK-22774][SQL][Test] Add compilation check into TPCDSQuerySuite Dec 13, 2017

gatorsmile mentioned this pull request Dec 13, 2017

[SPARK-22600][SQL][FOLLOW-UP] Fix a compilation error in TPCDS q75/q77 #19969

Closed

maropu reviewed Dec 13, 2017

View reviewed changes

initial commit

ace2944

viirya reviewed Dec 14, 2017

View reviewed changes

address review comments

1480ae3

kiszk force-pushed the SPARK-22774 branch from 19a0d2d to 1480ae3 Compare December 14, 2017 05:45

cloud-fan reviewed Dec 14, 2017

View reviewed changes

address review comments

7d2c599

maropu reviewed Dec 14, 2017

View reviewed changes

address review comment

dacb790

maropu reviewed Dec 14, 2017

View reviewed changes

cloud-fan reviewed Dec 14, 2017

View reviewed changes

address review comments

44abd2f

asfgit closed this in 606ae49 Dec 14, 2017

[SPARK-22774][SQL][Test] Add compilation check into TPCDSQuerySuite #19971

[SPARK-22774][SQL][Test] Add compilation check into TPCDSQuerySuite #19971

Uh oh!

Conversation

kiszk commented Dec 13, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

mgaido91 commented Dec 13, 2017

Uh oh!

kiszk commented Dec 13, 2017

Uh oh!

SparkQA commented Dec 13, 2017

Uh oh!

gatorsmile commented Dec 13, 2017

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!

maropu commented Dec 13, 2017

Uh oh!

JoshRosen commented Dec 14, 2017

Uh oh!

JoshRosen commented Dec 14, 2017

Uh oh!

cloud-fan commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Dec 14, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 14, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Dec 14, 2017

Uh oh!

SparkQA commented Dec 14, 2017

Uh oh!

SparkQA commented Dec 14, 2017

Uh oh!

cloud-fan commented Dec 14, 2017

Uh oh!

gatorsmile commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

kiszk Dec 14, 2017 •

edited

Loading

cloud-fan Dec 14, 2017 •

edited

Loading

gatorsmile commented Dec 14, 2017 •

edited

Loading