[SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option #12353

dongjoon-hyun · 2016-04-13T07:50:36Z

What changes were proposed in this pull request?

We currently disable codegen for CaseWhen if the number of branches is greater than 20 (in CaseWhen.MAX_NUM_CASES_FOR_CODEGEN). It would be better if this value is a non-public config defined in SQLConf.

How was this patch tested?

Pass the Jenkins tests (including a new testcase Support spark.sql.codegen.maxCaseBranches option)

SparkQA · 2016-04-13T07:53:42Z

Test build #55701 has finished for PR 12353 at commit 9a83c7f.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-04-13T07:56:41Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -52,6 +52,8 @@ case class ExprCode(var code: String, var isNull: String, var value: String)
 */
 class CodegenContext {

+  var conf: CatalystConf = null


this is really hacky -- i'd put this in the constructor and make it a val rather than a var. and maybe we can create a CodegenConf instead of reusing CatalystConf?

Oh, Sure. I'll change those things.

SparkQA · 2016-04-13T09:15:09Z

Test build #55702 has finished for PR 12353 at commit 6295de5.

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

SparkQA · 2016-04-13T10:17:25Z

Test build #55705 has finished for PR 12353 at commit 61b2dc3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class SimpleCodegenConf(maxCaseBranches: Int = 20) extends CodegenConf
- class CodegenContext(codegenConf: CodegenConf)

dongjoon-hyun · 2016-04-13T17:06:26Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala

@@ -620,7 +622,7 @@ abstract class CodeGenerator[InType <: AnyRef, OutType <: AnyRef] extends Loggin
   * expressions that don't support codegen
   */
  def newCodeGenContext(): CodegenContext = {
-    new CodegenContext
+    new CodegenContext(new SimpleCodegenConf)


Hmm.
CodegenContext had better be a parameter.
I will fix this.

I overlooked the purpose of this function. It's for expressions that don't support codegen. I replaced SimpleCodegenConf with EmptyCodegenConf.

/** * Create a new codegen context for expression evaluator, used to store those * expressions that don't support codegen */

rxin · 2016-04-13T17:12:07Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/codegen/CodegenConf.scala

+
+package org.apache.spark.sql.catalyst.expressions.codegen
+
+private[spark] trait CodegenConf {


maybe this can just be a class with a list of options, e.g.

class CodegenConf(val maxCaseBranches: Int)

The current relationship between SQLConf and catalyst module confs (CatalystConf, CodegenConf) uses trait. Should we change that manner consistently (including CatalystConf)? After this PR, we might have more conf classes in catalyst module in the future.

-private[sql] class SQLConf extends Serializable with CatalystConf with Logging { +private[sql] class SQLConf extends Serializable with CatalystConf with CodegenConf with Logging {

SparkQA · 2016-04-13T22:03:42Z

Test build #55740 has finished for PR 12353 at commit 2d7a509.

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

dongjoon-hyun · 2016-04-13T22:10:27Z

Hi, @rxin .
Now, this PR is ready to be reviewed.
Could you review this please?

dongjoon-hyun · 2016-04-14T08:59:44Z

Rebased to resolve conflicts.

SparkQA · 2016-04-14T10:22:26Z

Test build #55808 has finished for PR 12353 at commit fe8510a.

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

dongjoon-hyun · 2016-04-15T18:32:26Z

Rebased to resolve conflicts.

SparkQA · 2016-04-15T19:59:23Z

Test build #55943 has finished for PR 12353 at commit 6842e68.

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

rxin · 2016-04-16T03:33:05Z

@dongjoon-hyun what's your idea on how to update this?

dongjoon-hyun · 2016-04-16T08:15:51Z

Oh, sorry. I didn't say explicitly. I thought updating private[spark] trait CodegenConf into trait CodegenConf because CatalystConf changed like that.

dongjoon-hyun · 2016-04-16T19:03:41Z

Actually, it's not needed.
If there are some missing things to do, could you give me some advice?

rxin · 2016-04-16T21:47:58Z

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

@@ -305,7 +307,7 @@ case class WholeStageCodegen(child: SparkPlan) extends UnaryNode with CodegenSup
   * @return the tuple of the codegen context and the actual generated source.
   */
  def doCodeGen(): (CodegenContext, String) = {
-    val ctx = new CodegenContext
+    val ctx = new CodegenContext(SQLContext.getActive().get.conf)


Is this a problem on non-local mode? SQLContext.getActive is not available on the executors.

Oh, I haven't test it non-local mode. If then, is there any way to access the configuration of SQLContext in executors?

rxin · 2016-04-18T21:16:36Z

OK I think about this more. Actually to make this really work, we should just create two expressions, one for codegen version and the other for interpreted (default). And in the optimizer we switch to the codegen version if the number of branches is less than x.

cc @davies

dongjoon-hyun · 2016-04-18T21:35:20Z

I see. Thank you for the solution!

dongjoon-hyun · 2016-04-18T22:00:49Z

For optimizer, may I implement this in SimplifyConditionals?
Or, should I create another one like CaseWhenCodegen?

rxin · 2016-04-18T22:04:10Z

Might be best to create a OptimizeCodegen rule as the very last batch. We can add other things to that rule in the future.

dongjoon-hyun · 2016-04-18T22:04:47Z

Thanks. I will create OptimizeCodegen then.

…comments.

dongjoon-hyun · 2016-04-19T04:39:05Z

Hi, @rxin . Now, the PR is updated in the following ways.

CaseWhen split into three classes:
- CaseWhenBase: abstract base class
- CaseWhen: codegen fallback
- CaseWhenCodegen: codegen
OptimizeCodegen optimizer added:
- Replace CaseWhen into CaseWhenCodegen if branches.size < conf.maxCasesBranches.
CodegenConf is removed and CatalystConf has maxCasesBranches. (SQLConf does not changed.)
- Since Optimizer uses that configuration, I think CatalystConf becomes more proper place.

How do you think about item 3?

SparkQA · 2016-04-19T05:54:07Z

Test build #56191 has finished for PR 12353 at commit a9294bd.

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

rxin · 2016-04-19T06:09:43Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystConf.scala

@@ -29,6 +29,7 @@ trait CatalystConf {
  def groupByOrdinal: Boolean

  def optimizerMaxIterations: Int
+  def maxCaseBranches: Int


maxCaseBranchesForCodegen?

Thank you for quick review. Sure. And also maxCaseBranchesForCodegen in SQLConf.scala.

rxin · 2016-04-19T06:12:15Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

+case class CaseWhen(
+    val branches: Seq[(Expression, Expression)],
+    val elseValue: Option[Expression] = None)
+  extends CaseWhenBase(branches, elseValue) with CodegenFallback with Serializable {


maybe just have a toCodegen function that creates CaseWhenCodegen?

We can then remove object CaseWhenCodegen

That would be right. CaseWhenCodegen is always generated from CaseWhen.

rxin · 2016-04-19T06:15:09Z

cc @cloud-fan this change actually makes your other thing easier i think.

rxin · 2016-04-19T06:15:41Z

...talyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/conditionalExpressions.scala

@@ -242,6 +261,12 @@ object CaseWhen {
  }
 }

+/** Factory methods for CaseWhenCodegen. */
+object CaseWhenCodegen {


we can remove this given the above comment

cloud-fan · 2016-04-19T07:30:37Z

can we mark CodegenFallback.doCodeGen as final? so that we can guarantee that expressions implement CodegenFallback will always fallback.

rxin · 2016-04-19T07:34:36Z

I don't think we can do that unless we "fix" Literal.

dongjoon-hyun · 2016-04-19T08:02:16Z

Now, the followings are updated.

maxCaseBranches is renamed to maxCaseBranchesForCodegen.
object CaseWhenCodegen is removed.
CaseWhen has 'toCodegen` function.
3 testcases added: nested CaseWhen, multiple CaseWhen in one operator, multiple CaseWhen in multiple operators.

cloud-fan · 2016-04-19T08:36:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala

+ */
+case class OptimizeCodegen(conf: CatalystConf) extends Rule[LogicalPlan] {
+  def apply(plan: LogicalPlan): LogicalPlan = plan transformAllExpressions {
+    case e @ CaseWhen(branches, elseCase) if branches.size < conf.maxCaseBranchesForCodegen =>


nit: the elseCase is not used

Thank you, @cloud-fan !

cloud-fan · 2016-04-19T08:38:47Z

LGTM

dongjoon-hyun · 2016-04-19T08:44:45Z

Thank you for review, @cloud-fan . It's fixed.

SparkQA · 2016-04-19T09:19:40Z

Test build #56210 has finished for PR 12353 at commit 502bc61.

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

SparkQA · 2016-04-19T10:13:47Z

Test build #56212 has finished for PR 12353 at commit 9a2340c.

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

cloud-fan · 2016-04-19T13:40:20Z

thanks! merging to master!

dongjoon-hyun · 2016-04-19T16:52:05Z

Thank you for merging, @cloud-fan ! :)

dongjoon-hyun · 2016-04-19T16:53:40Z

Also, thank you so much for your direct guidance, @rxin .

rxin reviewed Apr 13, 2016
View reviewed changes

dongjoon-hyun reviewed Apr 13, 2016
View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option~~ [WIP][SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option Apr 13, 2016

rxin reviewed Apr 13, 2016
View reviewed changes

dongjoon-hyun changed the title ~~[WIP][SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option~~ [SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option Apr 13, 2016

dongjoon-hyun mentioned this pull request Apr 15, 2016

[SPARK-14677][SQL] Make the max number of iterations configurable for Catalyst #12434

Closed

rxin reviewed Apr 16, 2016
View reviewed changes

dongjoon-hyun added 3 commits April 18, 2016 21:05

[SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option

25ca987

Replace SimpleCodegenConf class with EmptyCodegenConf object and add …

b97d3ea

…comments.

Use OptimizeCodegen and CatalystConf.

a9294bd

rxin reviewed Apr 19, 2016
View reviewed changes

Address comments

502bc61

cloud-fan reviewed Apr 19, 2016
View reviewed changes

Replace unused matching variable.

9a2340c

asfgit closed this in 3d46d79 Apr 19, 2016

dongjoon-hyun deleted the SPARK-14577 branch May 12, 2016 00:59

gatorsmile mentioned this pull request May 29, 2016

[SPARK-15647] [SQL] Fix Boundary Cases in OptimizeCodegen Rule #13392

Closed


		package org.apache.spark.sql.catalyst.expressions.codegen

		private[spark] trait CodegenConf {

[SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option #12353

[SPARK-14577][SQL] Add spark.sql.codegen.maxCaseBranches config option #12353

Conversation

dongjoon-hyun commented Apr 13, 2016

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 13, 2016

SparkQA commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Apr 13, 2016

dongjoon-hyun commented Apr 13, 2016

dongjoon-hyun commented Apr 14, 2016

SparkQA commented Apr 14, 2016

dongjoon-hyun commented Apr 15, 2016

SparkQA commented Apr 15, 2016

rxin commented Apr 16, 2016

dongjoon-hyun commented Apr 16, 2016

dongjoon-hyun commented Apr 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxin commented Apr 18, 2016 • edited

dongjoon-hyun commented Apr 18, 2016

dongjoon-hyun commented Apr 18, 2016

rxin commented Apr 18, 2016

dongjoon-hyun commented Apr 18, 2016

dongjoon-hyun commented Apr 19, 2016

SparkQA commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxin commented Apr 19, 2016

Choose a reason for hiding this comment

cloud-fan commented Apr 19, 2016

rxin commented Apr 19, 2016

dongjoon-hyun commented Apr 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Apr 19, 2016

dongjoon-hyun commented Apr 19, 2016

SparkQA commented Apr 19, 2016

SparkQA commented Apr 19, 2016

cloud-fan commented Apr 19, 2016

dongjoon-hyun commented Apr 19, 2016

dongjoon-hyun commented Apr 19, 2016

rxin commented Apr 18, 2016 •

edited