Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kiszk wants to merge 5 commits into from
Closed

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ace2944
initial commit
kiszk Dec 13, 2017
1480ae3
address review comments
kiszk Dec 14, 2017
7d2c599
address review comments
kiszk Dec 14, 2017
dacb790
address review comment
kiszk Dec 14, 2017
44abd2f
address review comments
kiszk Dec 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 33 additions & 5 deletions sql/core/src/test/scala/org/apache/spark/sql/TPCDSQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ package org.apache.spark.sql

import org.scalatest.BeforeAndAfterAll

import org.apache.spark.internal.Logging
import org.apache.spark.sql.catalyst.expressions.codegen.{CodeFormatter, CodeGenerator}
import org.apache.spark.sql.catalyst.rules.RuleExecutor
import org.apache.spark.sql.catalyst.util.resourceToString
import org.apache.spark.sql.execution.{SparkPlan, WholeStageCodegenExec}
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.test.SharedSQLContext
import org.apache.spark.util.Utils
Expand All @@ -29,7 +32,7 @@ import org.apache.spark.util.Utils
* This test suite ensures all the TPC-DS queries can be successfully analyzed and optimized
* without hitting the max iteration threshold.
*/
class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfterAll {
class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfterAll with Logging {

@maropu maropu Dec 14, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: drop Logging and the import


// When Utils.isTesting is true, the RuleExecutor will issue an exception when hitting
// the max iteration of analyzer/optimizer batches.
Expand Down Expand Up @@ -348,13 +351,37 @@ class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfte
"q81", "q82", "q83", "q84", "q85", "q86", "q87", "q88", "q89", "q90",
"q91", "q92", "q93", "q94", "q95", "q96", "q97", "q98", "q99")

private def checkGeneratedCode(plan: SparkPlan): Unit = {
val codegenSubtrees = new collection.mutable.HashSet[WholeStageCodegenExec]()
plan foreach {
case s: WholeStageCodegenExec =>
codegenSubtrees += s
case s => s
}
codegenSubtrees.toSeq.foreach { subtree =>
val code = subtree.doCodeGen()._2
try {
// Just check the generated code can be properly compiled
CodeGenerator.compile(code)
} catch {
case e: Exception =>
logError(s"failed to compile: $e", e)

@maropu maropu Dec 14, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this log? I feel a bit meaningless.

@kiszk kiszk Dec 14, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
    }

@maropu maropu Dec 14, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cloud-fan cloud-fan Dec 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cloud-fan cloud-fan Dec 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can combine it with the exception error message

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

val msg =
s"Subtree:\n$subtree\n" +
s"Generated code:\n${CodeFormatter.format(code)}\n"
throw new Exception(msg, e)
}
}
}

tpcdsQueries.foreach { name =>
val queryString = resourceToString(s"tpcds/$name.sql",
classLoader = Thread.currentThread().getContextClassLoader)
test(name) {
withSQLConf(SQLConf.CROSS_JOINS_ENABLED.key -> "true") {
// Just check the plans can be properly generated
sql(queryString).queryExecution.executedPlan
// check the plans can be properly generated
val plan = sql(queryString).queryExecution.executedPlan
checkGeneratedCode(plan)
}
}
}
Expand All @@ -368,8 +395,9 @@ class TPCDSQuerySuite extends QueryTest with SharedSQLContext with BeforeAndAfte
val queryString = resourceToString(s"tpcds-modifiedQueries/$name.sql",
classLoader = Thread.currentThread().getContextClassLoader)
test(s"modified-$name") {
// Just check the plans can be properly generated
sql(queryString).queryExecution.executedPlan
// check the plans can be properly generated
val plan = sql(queryString).queryExecution.executedPlan
checkGeneratedCode(plan)
}
}
}