[SPARK-14338][SQL] Improve `SimplifyConditionals` rule to handle `null` in IF/CASEWHEN #12122

dongjoon-hyun · 2016-04-01T23:52:50Z

What changes were proposed in this pull request?

Currently, SimplifyConditionals handles true and false to optimize branches. This PR improves SimplifyConditionals to take advantage of null conditions for if and CaseWhen expressions, too.

Before

scala> sql("SELECT IF(null, 1, 0)").explain()
== Physical Plan ==
WholeStageCodegen
:  +- Project [if (null) 1 else 0 AS (IF(CAST(NULL AS BOOLEAN), 1, 0))#4]
:     +- INPUT
+- Scan OneRowRelation[]
scala> sql("select case when cast(null as boolean) then 1 else 2 end").explain()
== Physical Plan ==
WholeStageCodegen
:  +- Project [CASE WHEN null THEN 1 ELSE 2 END AS CASE WHEN CAST(NULL AS BOOLEAN) THEN 1 ELSE 2 END#14]
:     +- INPUT
+- Scan OneRowRelation[]

After

scala> sql("SELECT IF(null, 1, 0)").explain()
== Physical Plan ==
WholeStageCodegen
:  +- Project [0 AS (IF(CAST(NULL AS BOOLEAN), 1, 0))#4]
:     +- INPUT
+- Scan OneRowRelation[]
scala> sql("select case when cast(null as boolean) then 1 else 2 end").explain()
== Physical Plan ==
WholeStageCodegen
:  +- Project [2 AS CASE WHEN CAST(NULL AS BOOLEAN) THEN 1 ELSE 2 END#4]
:     +- INPUT
+- Scan OneRowRelation[]

Hive

hive> select if(null,1,2);
OK
2
hive> select case when cast(null as boolean) then 1 else 2 end;
OK
2

How was this patch tested?

Pass the Jenkins tests (including new extended test cases).

dongjoon-hyun · 2016-04-02T00:16:41Z

deep-review this please

DeepSparkBot · 2016-04-02T00:28:03Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/literals.scala

@@ -33,6 +33,8 @@ object Literal {

  val FalseLiteral: Literal = Literal(false, BooleanType)

+  val NullLiteral: Literal = Literal(null, NullType)


Detected duplicate pattern.

We should replace Literal(null, NullType) in L55 with NullLiteral.

DeepSparkBot · 2016-04-02T00:37:49Z

@dongjoon-hyun Review complete. No major issues found.

This matches Hive semantics, where SELECT if(null, 1, 2) returns 2. However, this prevents a future optimization where if(anything, TrueLiteral, FalseLiteral) can be simplified to anything.

This requires human decision. @JoshRosen @sameeragarwal @yhuai Please advise.

SparkQA · 2016-04-02T01:13:50Z

Test build #54743 has finished for PR 12122 at commit 22bb31f.

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

SparkQA · 2016-04-02T02:10:44Z

Test build #54747 has finished for PR 12122 at commit 512a45f.

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

dongjoon-hyun · 2016-04-02T03:58:46Z

Rebased and squashed.

SparkQA · 2016-04-02T05:12:29Z

Test build #54758 has finished for PR 12122 at commit dcbb0ea.

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

dongjoon-hyun · 2016-04-02T05:30:14Z

It's strange.
org.apache.spark.sql.execution.streaming.state.StateStoreSuite.maintenance test fails here and at my other PR frequently.

SparkQA · 2016-04-02T06:01:37Z

Test build #2729 has finished for PR 12122 at commit dcbb0ea.

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

rxin · 2016-04-02T07:08:00Z

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

@@ -773,17 +773,24 @@ object BooleanSimplification extends Rule[LogicalPlan] with PredicateHelper {
 * Simplifies conditional expressions (if / case).
 */
 object SimplifyConditionals extends Rule[LogicalPlan] with PredicateHelper {
+  def falseOrNullLiteral(e: Expression): Boolean = e match {


private def

rxin · 2016-04-02T07:09:09Z

LGTM pending Jenkins

dongjoon-hyun · 2016-04-02T07:51:59Z

Thank you, @rxin .
I fixed that to private def, too.

SparkQA · 2016-04-02T08:32:02Z

Test build #2730 has finished for PR 12122 at commit dcbb0ea.

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

SparkQA · 2016-04-02T09:10:49Z

Test build #54768 has finished for PR 12122 at commit d8c3f38.

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

dongjoon-hyun · 2016-04-02T10:02:40Z

The last failure is also org.apache.spark.sql.execution.streaming.state.StateStoreSuite.maintenance. It's irrelevant to this PR.

…l` in IF/CASEWHEN

dongjoon-hyun · 2016-04-02T18:16:52Z

Rebased.

SparkQA · 2016-04-02T19:35:31Z

Test build #54780 has finished for PR 12122 at commit 2870d4e.

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

SparkQA · 2016-04-02T21:11:32Z

Test build #2733 has finished for PR 12122 at commit 2870d4e.

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

rxin · 2016-04-03T00:48:37Z

Thanks - merging in master.

yhuai · 2016-04-03T02:21:48Z

@dongjoon-hyun Can you take a look at the scala 2.10 build?

rxin · 2016-04-03T06:06:10Z

I think I fixed it just now.

dongjoon-hyun · 2016-04-03T07:48:55Z

Oh, sorry for breaking Scala 2.10 build and late response, @yhuai and @rxin .
Thank you so much for hot fix, @rxin !

DeepSparkBot reviewed Apr 2, 2016
View reviewed changes

rxin reviewed Apr 2, 2016
View reviewed changes

dongjoon-hyun added 2 commits April 2, 2016 11:16

[SPARK-14338][SQL] Improve SimplifyConditionals rule to handle `nul…

506a477

…l` in IF/CASEWHEN

Make private def.

2870d4e

asfgit closed this in f705037 Apr 3, 2016

dongjoon-hyun deleted the SPARK-14338 branch May 12, 2016 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-14338][SQL] Improve `SimplifyConditionals` rule to handle `null` in IF/CASEWHEN #12122

[SPARK-14338][SQL] Improve `SimplifyConditionals` rule to handle `null` in IF/CASEWHEN #12122

dongjoon-hyun commented Apr 1, 2016

dongjoon-hyun commented Apr 2, 2016

DeepSparkBot Apr 2, 2016

DeepSparkBot commented Apr 2, 2016

SparkQA commented Apr 2, 2016

SparkQA commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

rxin Apr 2, 2016

rxin commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

SparkQA commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

SparkQA commented Apr 2, 2016

rxin commented Apr 3, 2016

yhuai commented Apr 3, 2016

rxin commented Apr 3, 2016

dongjoon-hyun commented Apr 3, 2016

		@@ -33,6 +33,8 @@ object Literal {

		val FalseLiteral: Literal = Literal(false, BooleanType)

		val NullLiteral: Literal = Literal(null, NullType)

[SPARK-14338][SQL] Improve SimplifyConditionals rule to handle null in IF/CASEWHEN #12122

[SPARK-14338][SQL] Improve SimplifyConditionals rule to handle null in IF/CASEWHEN #12122

Conversation

dongjoon-hyun commented Apr 1, 2016

What changes were proposed in this pull request?

How was this patch tested?

dongjoon-hyun commented Apr 2, 2016

DeepSparkBot Apr 2, 2016

Choose a reason for hiding this comment

DeepSparkBot commented Apr 2, 2016

SparkQA commented Apr 2, 2016

SparkQA commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

rxin Apr 2, 2016

Choose a reason for hiding this comment

rxin commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

SparkQA commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

dongjoon-hyun commented Apr 2, 2016

SparkQA commented Apr 2, 2016

SparkQA commented Apr 2, 2016

rxin commented Apr 3, 2016

yhuai commented Apr 3, 2016

rxin commented Apr 3, 2016

dongjoon-hyun commented Apr 3, 2016

[SPARK-14338][SQL] Improve `SimplifyConditionals` rule to handle `null` in IF/CASEWHEN #12122

[SPARK-14338][SQL] Improve `SimplifyConditionals` rule to handle `null` in IF/CASEWHEN #12122