[SPARK-21845] [SQL] Make codegen fallback of expressions configurable #19062

gatorsmile · 2017-08-26T22:18:27Z

What changes were proposed in this pull request?

We should make codegen fallback of expressions configurable. So far, it is always on. We might hide it when our codegen have compilation bugs. Thus, we should also disable the codegen fallback when running test cases.

How was this patch tested?

Added test cases

gatorsmile · 2017-08-26T22:19:21Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala

      GeneratePredicate.generate(expression, inputSchema)
    } catch {
-      case e @ (_: JaninoRuntimeException | _: CompileException)
-          if sqlContext == null || sqlContext.conf.wholeStageFallback =>


Because sqlContext is always null when running it in executors, and thus, this always return true.

Better to put this comment in https://github.com/apache/spark/pull/19062/files#diff-b9f96d092fb3fea76bcf75e016799678R57?

Removing the null check here makes sense although this means existing spark jobs that were previously switching to the non-codegen version even with sqlContext.conf.wholeStageFallback = false will now start failing at runtime (perhaps, rightly so). Might be worth calling this out in the 2.3 release notes and/or the migration guide.

gatorsmile · 2017-08-26T22:20:37Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/test/TestHive.scala

      "TestSQLContext",
      new SparkConf()
        .set("spark.sql.test", "")
+        .set(SQLConf.CODEGEN_FALLBACK.key, "false")


Turn it false to ensure it does not hide the actual bugs of our expression codegen that causes compilation falure.

gatorsmile · 2017-08-26T22:21:37Z

cc @kiszk @cloud-fan @zsxwing

kiszk · 2017-08-27T00:01:06Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala

  final val sqlContext = SparkSession.getActiveSession.map(_.sqlContext).orNull

+  // whether we should fallback when hitting compilation errors caused by codegen
+  private val codeGenFallBack = sqlContext == null || sqlContext.conf.codegenFallback


Is it better to add !Utils.isTesting && or to drop !Utils.isTesting && from WholeStageCodegenExec to make these conditions consistent?

Originally, I did it like what you said. However, if using that approach, I need to remove the test case. Then, I think we might just keep using the codegenFallback for controlling it.

SparkQA · 2017-08-27T01:07:31Z

Test build #81158 has finished for PR 19062 at commit 9f3a1cd.

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

dongjoon-hyun

+1, LGTM.

heary-cao · 2017-08-27T16:59:02Z

LGTM

maropu · 2017-08-28T02:44:25Z

LGTM

gatorsmile · 2017-08-29T19:50:11Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala

-  // sqlContext will be null when we are being deserialized on the slaves.  In this instance
-  // the value of subexpressionEliminationEnabled will be set by the deserializer after the
-  // constructor has run.
-  val subexpressionEliminationEnabled: Boolean = if (sqlContext != null) {


cc @marmbrus @cloud-fan Does this sound OK? Currently, our codebase does not check nullability in most places. This value will be always not null when we initialize the value.

SparkQA · 2017-08-29T20:26:35Z

Test build #81229 has finished for PR 19062 at commit 26dcbd6.

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

SparkQA · 2017-08-29T22:54:27Z

Test build #81230 has finished for PR 19062 at commit 4f39a52.

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

gatorsmile · 2017-08-30T03:59:18Z

Thanks! Merging to master.

srowen · 2017-08-30T10:42:56Z

I think this makes the master build fail @gatorsmile :

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

*** RUN ABORTED ***
  java.lang.RuntimeException: Unable to load a Suite class that was discovered in the runpath: org.apache.spark.sql.execution.PlannerSuite
  at org.scalatest.tools.DiscoverySuite$.getSuiteInstance(DiscoverySuite.scala:84)
  at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:38)
  at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:37)
  at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
  at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:234)
  at scala.collection.Iterator$class.foreach(Iterator.scala:893)
  at scala.collection.AbstractIterator.foreach(Iterator.scala:1336)
  at scala.collection.IterableLike$class.foreach(IterableLike.scala:72)
  at scala.collection.AbstractIterable.foreach(Iterable.scala:54)
  at scala.collection.TraversableLike$class.map(TraversableLike.scala:234)
  ...
  Cause: java.lang.NullPointerException:
  at org.apache.spark.sql.execution.SparkPlan.<init>(SparkPlan.scala:60)
  at org.apache.spark.sql.execution.DummySparkPlan.<init>(PlannerSuite.scala:638)
  at org.apache.spark.sql.execution.PlannerSuite.<init>(PlannerSuite.scala:484)
  at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
  at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
  at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
  at java.lang.reflect.Constructor.newInstance(Constructor.java:422)
  at java.lang.Class.newInstance(Class.java:442)
  at org.scalatest.tools.DiscoverySuite$.getSuiteInstance(DiscoverySuite.scala:69)
  at org.scalatest.tools.DiscoverySuite$$anonfun$1.apply(DiscoverySuite.scala:38)

gatorsmile · 2017-08-30T16:06:19Z

Thanks! Let me first revert this PR.

fix.

9f3a1cd

gatorsmile commented Aug 26, 2017

View reviewed changes

kiszk reviewed Aug 27, 2017

View reviewed changes

dongjoon-hyun approved these changes Aug 27, 2017

View reviewed changes

fix

26dcbd6

gatorsmile commented Aug 29, 2017

View reviewed changes

fix

4f39a52

maropu mentioned this pull request Aug 30, 2017

[SPARK-21871][SQL] Check actual bytecode size when compiling generated code #19083

Closed

asfgit closed this in 3d0e174 Aug 30, 2017

[SPARK-21845] [SQL] Make codegen fallback of expressions configurable #19062

[SPARK-21845] [SQL] Make codegen fallback of expressions configurable #19062

Uh oh!

Conversation

gatorsmile commented Aug 26, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile Aug 26, 2017

Choose a reason for hiding this comment

Uh oh!

maropu Aug 28, 2017

Choose a reason for hiding this comment

Uh oh!

sameeragarwal Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

gatorsmile Aug 26, 2017

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Aug 26, 2017

Uh oh!

kiszk Aug 27, 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!

gatorsmile Aug 27, 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!

kiszk Aug 27, 2017

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 27, 2017

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

heary-cao commented Aug 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Aug 28, 2017

Uh oh!

gatorsmile Aug 29, 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!

SparkQA commented Aug 29, 2017

Uh oh!

SparkQA commented Aug 29, 2017

Uh oh!

gatorsmile commented Aug 30, 2017

Uh oh!

srowen commented Aug 30, 2017

Uh oh!

gatorsmile commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kiszk Aug 27, 2017 •

edited

Loading

gatorsmile Aug 27, 2017 •

edited

Loading

heary-cao commented Aug 27, 2017 •

edited

Loading

gatorsmile Aug 29, 2017 •

edited

Loading