[SPARK-24892] [SQL] Simplify `CaseWhen` to `If` when there is only one branch #21850

dbtsai · 2018-07-23T18:37:18Z

What changes were proposed in this pull request?

After the rule of removing the unreachable branches, it could be only one branch left. In this situation, CaseWhen can be converted to If to do further optimization.

How was this patch tested?

Tests added.

dongjoon-hyun · 2018-07-23T21:14:55Z

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

+        val cond = branches.head._1
+        val trueValue = branches.head._2
+        val falseValue = elseValue.getOrElse(Literal(null, trueValue.dataType))
+        If(cond, trueValue, falseValue)


Can we simplify more like the following pattern?

- case CaseWhen(branches, elseValue) if branches.length == 1 => - val cond = branches.head._1 - val trueValue = branches.head._2 - val falseValue = elseValue.getOrElse(Literal(null, trueValue.dataType)) - If(cond, trueValue, falseValue) + case e @ CaseWhen((cond, branchValue) :: Nil, elseValue) => + If(cond, branchValue, elseValue.getOrElse(Literal(null, e.dataType)))

Done. Thanks!

SparkQA · 2018-07-23T22:41:44Z

Test build #93456 has finished for PR 21850 at commit 18e2d8d.

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

dbtsai · 2018-07-23T23:37:57Z

@cloud-fan and @gatorsmile

SparkQA · 2018-07-24T00:30:49Z

Test build #93462 has finished for PR 21850 at commit a9c97ce.

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

dbtsai · 2018-07-24T01:21:56Z

retest this please

SparkQA · 2018-07-24T04:23:27Z

Test build #93468 has finished for PR 21850 at commit a9c97ce.

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

dbtsai · 2018-07-24T05:46:29Z

retest this please

HyukjinKwon · 2018-07-24T06:57:29Z

retest this please

SparkQA · 2018-07-24T07:05:02Z

Test build #93484 has finished for PR 21850 at commit a9c97ce.

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

HyukjinKwon · 2018-07-24T08:46:40Z

retest this please

SparkQA · 2018-07-24T12:45:59Z

Test build #93492 has finished for PR 21850 at commit a9c97ce.

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

viirya · 2018-07-24T17:50:51Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalSuite.scala

@@ -61,7 +61,17 @@ class SimplifyConditionalSuite extends PlanTest with PredicateHelper {
    // i.e. removing branches whose conditions are always false
    assertEquivalent(
      CaseWhen(unreachableBranch :: normalBranch :: unreachableBranch :: nullBranch :: Nil, None),
-      CaseWhen(normalBranch :: Nil, None))
+      If(normalBranch._1, normalBranch._2, Literal(null, normalBranch._2.dataType)))


Can we add an extra branch into this test, so it won't be optimized to single branch CaseWhen? Otherwise this test is changed from its original purpose.

gatorsmile · 2018-07-24T18:22:47Z

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

@@ -414,6 +414,9 @@ object SimplifyConditionals extends Rule[LogicalPlan] with PredicateHelper {
        // these branches can be pruned away
        val (h, t) = branches.span(_._1 != TrueLiteral)
        CaseWhen( h :+ t.head, None)
+
+      case CaseWhen((cond, branchValue) :: Nil, elseValue) =>
+        If(cond, branchValue, elseValue.getOrElse(Literal(null, branchValue.dataType)))


Could you post the difference of generated JAVA code?

Before:

== Parsed Logical Plan == 'Project [CASE WHEN isnull('a) THEN 1 END AS col1#181] +- 'UnresolvedRelation == Optimized Logical Plan == Project [CASE WHEN isnull(a#182) THEN 1 END AS col1#181] +- Relation[a#182] parquet

Generated Java code

/* 043 */ protected void processNext() throws java.io.IOException { /* 044 */ if (scan_mutableStateArray_1[0] == null) { /* 045 */ scan_nextBatch_0(); /* 046 */ } /* 047 */ while (scan_mutableStateArray_1[0] != null) { /* 048 */ int scan_numRows_0 = scan_mutableStateArray_1[0].numRows(); /* 049 */ int scan_localEnd_0 = scan_numRows_0 - scan_batchIdx_0; /* 050 */ for (int scan_localIdx_0 = 0; scan_localIdx_0 < scan_localEnd_0; scan_localIdx_0++) { /* 051 */ int scan_rowIdx_0 = scan_batchIdx_0 + scan_localIdx_0; /* 052 */ byte project_caseWhenResultState_0 = -1; /* 053 */ do { /* 054 */ boolean scan_isNull_0 = scan_mutableStateArray_2[0].isNullAt(scan_rowIdx_0); /* 055 */ int scan_value_0 = scan_isNull_0 ? -1 : (scan_mutableStateArray_2[0].getInt(scan_rowIdx_0)); /* 056 */ if (!false && scan_isNull_0) { /* 057 */ project_caseWhenResultState_0 = (byte)(false ? 1 : 0); /* 058 */ project_project_value_0_0 = 1; /* 059 */ continue; /* 060 */ } /* 061 */ /* 062 */ } while (false); /* 063 */ // TRUE if any condition is met and the result is null, or no any condition is met. /* 064 */ final boolean project_isNull_0 = (project_caseWhenResultState_0 != 0); /* 065 */ scan_mutableStateArray_3[1].reset(); /* 066 */ /* 067 */ scan_mutableStateArray_3[1].zeroOutNullBytes(); /* 068 */ /* 069 */ if (project_isNull_0) { /* 070 */ scan_mutableStateArray_3[1].setNullAt(0); /* 071 */ } else { /* 072 */ scan_mutableStateArray_3[1].write(0, project_project_value_0_0); /* 073 */ } /* 074 */ append((scan_mutableStateArray_3[1].getRow())); /* 075 */ if (shouldStop()) { scan_batchIdx_0 = scan_rowIdx_0 + 1; return; } /* 076 */ } /* 077 */ scan_batchIdx_0 = scan_numRows_0; /* 078 */ scan_mutableStateArray_1[0] = null; /* 079 */ scan_nextBatch_0(); /* 080 */ } /* 081 */ ((org.apache.spark.sql.execution.metric.SQLMetric) references[1] /* scanTime */).add(scan_scanTime_0 / (1000 * 1000)); /* 082 */ scan_scanTime_0 = 0; /* 083 */ }

After:

== Parsed Logical Plan == 'Project [CASE WHEN isnull('a) THEN 1 END AS b#186] +- 'UnresolvedRelation `tddddd` == Optimized Logical Plan == Project [if (isnull(a#187)) 1 else null AS b#186] +- Relation[a#187,b#188] parquet

Generated Java code:

/* 042 */ protected void processNext() throws java.io.IOException { /* 043 */ if (scan_mutableStateArray_1[0] == null) { /* 044 */ scan_nextBatch_0(); /* 045 */ } /* 046 */ while (scan_mutableStateArray_1[0] != null) { /* 047 */ int scan_numRows_0 = scan_mutableStateArray_1[0].numRows(); /* 048 */ int scan_localEnd_0 = scan_numRows_0 - scan_batchIdx_0; /* 049 */ for (int scan_localIdx_0 = 0; scan_localIdx_0 < scan_localEnd_0; scan_localIdx_0++) { /* 050 */ int scan_rowIdx_0 = scan_batchIdx_0 + scan_localIdx_0; /* 051 */ boolean scan_isNull_0 = scan_mutableStateArray_2[0].isNullAt(scan_rowIdx_0); /* 052 */ int scan_value_0 = scan_isNull_0 ? -1 : (scan_mutableStateArray_2[0].getInt(scan_rowIdx_0)); /* 053 */ boolean project_isNull_0 = false; /* 054 */ int project_value_0 = -1; /* 055 */ if (!false && scan_isNull_0) { /* 056 */ project_isNull_0 = false; /* 057 */ project_value_0 = 1; /* 058 */ } else { /* 059 */ project_isNull_0 = true; /* 060 */ project_value_0 = -1; /* 061 */ } /* 062 */ scan_mutableStateArray_3[1].reset(); /* 063 */ /* 064 */ scan_mutableStateArray_3[1].zeroOutNullBytes(); /* 065 */ /* 066 */ if (project_isNull_0) { /* 067 */ scan_mutableStateArray_3[1].setNullAt(0); /* 068 */ } else { /* 069 */ scan_mutableStateArray_3[1].write(0, project_value_0); /* 070 */ } /* 071 */ append((scan_mutableStateArray_3[1].getRow())); /* 072 */ if (shouldStop()) { scan_batchIdx_0 = scan_rowIdx_0 + 1; return; } /* 073 */ } /* 074 */ scan_batchIdx_0 = scan_numRows_0; /* 075 */ scan_mutableStateArray_1[0] = null; /* 076 */ scan_nextBatch_0(); /* 077 */ } /* 078 */ ((org.apache.spark.sql.execution.metric.SQLMetric) references[1] /* scanTime */).add(scan_scanTime_0 / (1000 * 1000)); /* 079 */ scan_scanTime_0 = 0; /* 080 */ }

Look like not much difference in term of performance, but If primitive has more opportunities for further optimization.

For CaseWhen case, looks like there is an extra do while loop?

Also, CaseWhen has additional project_project_value_0_0 at outside.

/* 006 */ final class GeneratedIteratorForCodegenStage1 extends org.apache.spark.sql.execution.BufferedRowIterator { /* 007 */ private Object[] references; /* 008 */ private scala.collection.Iterator[] inputs; /* 009 */ private scala.collection.Iterator inputadapter_input_0; /* 010 */ private int project_project_value_0_0;

viirya · 2018-07-24T23:49:35Z

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

+
+      case CaseWhen(branches, elseValue) if branches.length == 1 =>
+        // Using pattern matching like `CaseWhen((cond, branchValue) :: Nil, elseValue)` will not
+        // work since the implementation of `branches` can be `ArrayBuffer`. A full test is in


Just curious, when is itArrayBuffer?

sql("select case when a is null then 1 end col1 from t") will create branches with ArrayBuffer implementation.

We can change it to immutable List to avoid confusion.

How about:

case CaseWhen(Seq((cond, trueValue)), elseValue) =>

+1 for @ueshin 's suggestion. And, sorry for this trouble, @dbtsai . :)

@ueshin thanks! The code is much more cleaner.

SparkQA · 2018-07-25T00:57:08Z

Test build #93521 has finished for PR 21850 at commit 59fada7.

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

dbtsai · 2018-07-25T00:58:44Z

retest this please

SparkQA · 2018-07-25T04:53:29Z

Test build #93525 has finished for PR 21850 at commit 59fada7.

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

viirya · 2018-07-25T05:33:40Z

LGTM

gatorsmile · 2018-07-25T07:40:23Z

Personally, I do not think we need this extra case.

If primitive has more opportunities for further optimization.

Could you explain more?

dongjoon-hyun · 2018-07-25T16:14:07Z

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

+
+      checkAnswer(plan1, Row(null) :: Row(1) :: Row(null) :: Nil)
+      comparePlans(plan1.queryExecution.optimizedPlan, plan2.queryExecution.optimizedPlan)
+    }


Thank you for adding this higher level test, too.

dongjoon-hyun

+1, LGTM.

SparkQA · 2018-07-25T18:56:06Z

Test build #93554 has finished for PR 21850 at commit e2b0e96.

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

viirya · 2018-07-25T19:00:10Z

retest this please.

dbtsai · 2018-07-25T22:39:17Z

@gatorsmile All the new rules added into If should always have CaseWhen version.

But there will be time that we only add If version, or it only makes sense to have If version.

For example, the new rule that short-circuiting the If when both branches are the same is not yet in CaseWhen. Another rule I am working on is when a conditional expression in Filter or Join, if any of the output of the branches contains Literal(null, _), it can be replaced by FalseLiteral. I only implemented for If so far for our use-case.

SparkQA · 2018-07-25T22:39:54Z

Test build #93564 has finished for PR 21850 at commit e2b0e96.

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

cloud-fan · 2018-07-26T15:39:54Z

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

@@ -414,6 +414,9 @@ object SimplifyConditionals extends Rule[LogicalPlan] with PredicateHelper {
        // these branches can be pruned away
        val (h, t) = branches.span(_._1 != TrueLiteral)
        CaseWhen( h :+ t.head, None)
+
+      case CaseWhen(Seq((cond, trueValue)), elseValue) =>
+        If(cond, trueValue, elseValue.getOrElse(Literal(null, trueValue.dataType)))


I don't think If is faster than CaseWhen, can you explain more about "further optimization"?

The generated Java code is slightly simpler, but I agree there should not have any performance gain. Being said that, once CaseWhen is converted into If, this condition expression will be benefited from the optimization rules in If which may not be implemented for CaseWhen case.

optimization rules in If which may not be implemented for CaseWhen case.

shall we just implement more optimizer rules for CASE WHEN to cover all the cases?

Let's revisit this PR later, and we should always try to add CASE WHEN version for parity.

Here is the one for case when.
#21852

SparkQA · 2020-08-29T08:56:48Z

Test build #128016 has finished for PR 21850 at commit e2b0e96.

This patch fails Scala style tests.
This patch does not merge cleanly.
This patch adds no public classes.

SparkQA · 2020-08-31T04:26:37Z

Test build #128070 has finished for PR 21850 at commit dd584a0.

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

dbtsai · 2020-08-31T17:25:19Z

...est/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceNullWithFalseInPredicateSuite.scala

@@ -200,13 +200,15 @@ class ReplaceNullWithFalseInPredicateSuite extends PlanTest {

  test("inability to replace null in non-boolean values of CaseWhen") {
    val nestedCaseWhen = CaseWhen(


Those tests are modified and one branch is added in CaseWhen to avoid new rule converting CaseWhen to If for better test coverage.

viirya · 2020-08-31T19:46:31Z

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

@@ -505,6 +505,9 @@ object SimplifyConditionals extends Rule[LogicalPlan] with PredicateHelper {
        } else {
          e.copy(branches = branches.take(i).map(branch => (branch._1, elseValue)))
        }
+
+      case CaseWhen(Seq((cond, trueValue)), elseValue) =>


Is it better we limit to BooleanType case? I.e.,

case cw @ CaseWhen(Seq((cond, trueValue)), elseValue) if cw.dataType == BooleanType =>

The reason is because mostly the further optimization comes from #29567, and it is for boolean type case only.

Or just rewrite it similarly like #29567?

case CaseWhen(Seq((cond, l @ Literal(null, _))), FalseLiteral) if !cond.nullable => ... case CaseWhen(Seq((cond, l @ Literal(null, _))), TrueLiteral) if !cond.nullable => ...

SparkQA · 2020-08-31T20:58:45Z

Test build #128112 has finished for PR 21850 at commit d34e908.

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

github-actions · 2020-12-10T00:48:06Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

dongjoon-hyun reviewed Jul 23, 2018

View reviewed changes

viirya reviewed Jul 24, 2018

View reviewed changes

gatorsmile reviewed Jul 24, 2018

View reviewed changes

viirya reviewed Jul 24, 2018

View reviewed changes

dongjoon-hyun reviewed Jul 25, 2018

View reviewed changes

dongjoon-hyun approved these changes Jul 25, 2018

View reviewed changes

cloud-fan reviewed Jul 26, 2018

View reviewed changes

dbtsai closed this Apr 5, 2019

dbtsai mentioned this pull request Aug 29, 2020

[SPARK-32721][SQL] Simplify if clauses with null and boolean #29567

Closed

dbtsai reopened this Aug 29, 2020

tanelk mentioned this pull request Aug 29, 2020

[SPARK-32727][SQL] Replace CaseWhen with If when there is only one case #29570

Closed

remove casewhen

dd584a0

dbtsai force-pushed the remove-case-when branch from e2b0e96 to dd584a0 Compare August 31, 2020 03:47

probot-autolabeler bot added the SQL label Aug 31, 2020

fix test

d34e908

dbtsai commented Aug 31, 2020

View reviewed changes

viirya reviewed Aug 31, 2020

View reviewed changes

github-actions bot added the Stale label Dec 10, 2020

github-actions bot closed this Dec 11, 2020

cloud-fan mentioned this pull request Dec 28, 2020

[SPARK-33884][SQL] Simplify CaseWhenclauses with (true and false) and (false and true) #30898

Closed

		@@ -200,13 +200,15 @@ class ReplaceNullWithFalseInPredicateSuite extends PlanTest {

		test("inability to replace null in non-boolean values of CaseWhen") {
		val nestedCaseWhen = CaseWhen(

[SPARK-24892] [SQL] Simplify CaseWhen to If when there is only one branch #21850

[SPARK-24892] [SQL] Simplify CaseWhen to If when there is only one branch #21850

Conversation

dbtsai commented Jul 23, 2018

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Jul 23, 2018

dbtsai commented Jul 23, 2018

SparkQA commented Jul 24, 2018

dbtsai commented Jul 24, 2018

SparkQA commented Jul 24, 2018

dbtsai commented Jul 24, 2018

HyukjinKwon commented Jul 24, 2018

SparkQA commented Jul 24, 2018

HyukjinKwon commented Jul 24, 2018

SparkQA commented Jul 24, 2018

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

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

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 Jul 25, 2018

dbtsai commented Jul 25, 2018

SparkQA commented Jul 25, 2018

viirya commented Jul 25, 2018

gatorsmile commented Jul 25, 2018

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented Jul 25, 2018

viirya commented Jul 25, 2018

dbtsai commented Jul 25, 2018

SparkQA commented Jul 25, 2018

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 Aug 29, 2020

SparkQA commented Aug 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 31, 2020

github-actions bot commented Dec 10, 2020

[SPARK-24892] [SQL] Simplify `CaseWhen` to `If` when there is only one branch #21850

[SPARK-24892] [SQL] Simplify `CaseWhen` to `If` when there is only one branch #21850