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

sunchao · 2020-08-28T02:12:54Z

What changes were proposed in this pull request?

The following if clause:

if(p, null, false)

can be simplified to:

and(p, null)

Similarly, the clause:

if(p, null, true)

can be simplified to

or(not(p), null)

iff the predicate p is non-nullable, i.e., can be evaluated to either true or false, but not null.

Why are the changes needed?

Converting if to or/and clauses can better push filters down.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit tests.

SparkQA · 2020-08-28T07:05:01Z

Test build #127976 has finished for PR 29567 at commit f9e87f0.

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

tanelk · 2020-08-28T07:30:44Z

Are you sure these rules are correct?
The first one seems to fail if p is null
And the seconds one seems to fail on all 3 values

I tried a quick test and 4 out of 6 of these failed.

  val nullLiteral = Literal(null, BooleanType)

  Seq(Literal.TrueLiteral, Literal.FalseLiteral, nullLiteral).foreach(p => {
    assertEquivalent(If(p, nullLiteral, Literal.FalseLiteral), And(p, nullLiteral))
    assertEquivalent(If(p, nullLiteral, Literal.TrueLiteral), Or(p, nullLiteral))
  })

  private def assertEquivalent(e1: Expression, e2: Expression): Unit = {
    test("SPARK-32721: " + e1.toString) {
      checkEvaluation (e1, e2.eval (EmptyRow) )
    }
  }

It seem that there might be some sort of equivalence here, but I'm not 100% sure how it should look like.

maropu · 2020-08-28T07:35:26Z

I don't look into it yet, but the motivation is the same with #27231 ?

sunchao · 2020-08-28T08:18:51Z

Thanks for checking. I made a few mistakes in the draft PR. Firstly, the second rule should be:

if(p, null, true) => or(not(p), null)

Secondly, the predicate p should be deterministic and evaluate to either true or false, but not null. I'll see how we can put restriction that, and add more tests.

Since three-valued logic has the following properties:

a	b	a and b	a or b
true	null	null	true
false	null	false	null

I think the above should be semantically equivalent.

tanelk · 2020-08-28T10:53:20Z

I think it should be correct now, but it would be nice to have some checks in the tests.

Are there equivalent cases for if(p, false, null) and if(p, true, null)?

Perhaps instead of a new Rule the cases could be added to the existing SimplifyConditionals?

HyukjinKwon · 2020-08-28T12:09:28Z

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

+  val nullLiteral = Literal(null, BooleanType)
+  def apply(plan: LogicalPlan): LogicalPlan = plan transformAllExpressions {
+    case If(p, Literal(null, _), FalseLiteral) => And(p, nullLiteral)
+    case If(p, Literal(null, _), TrueLiteral) => Or(p, nullLiteral)


Is it common enough to write an if with a null with returning a bool? Adding a rule isn't free and it gives some overhead to all the queries basically during the planning. I would avoid doing it if it's pretty unlikely.

How about merging these to the existing SimplifyConditionals as mentioned by @tanelk above? it will then just be a few more pattern matchings, right?

SparkQA · 2020-08-28T17:53:58Z

Test build #127998 has finished for PR 29567 at commit e4d929c.

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

SparkQA · 2020-08-28T21:42:23Z

Test build #127999 has finished for PR 29567 at commit bb7c38c.

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

SparkQA · 2020-08-29T01:34:48Z

Test build #128002 has finished for PR 29567 at commit cc66198.

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

sunchao · 2020-08-29T02:06:17Z

cc @dbtsai @viirya

dbtsai · 2020-08-29T08:47:30Z

This will be a very useful optimization to turn if causes into expressions that can be pushdown. For CaseWhen if there is only one branch, we can convert it into If. A PR awhile ago #21850 implemented this optimization but didn't end up merging into master because at that time, CaseWhen and If have not much performance difference. Maybe we can revisit that again.

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

viirya · 2020-08-30T07:08:00Z

...lyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala

-      testRelationWithData.where(If('e.isNull, Literal.create(null, BooleanType), false)).analyze)
+      testRelationWithData.where(And(Literal(null, BooleanType), 'e.isNull)).analyze)


Although I think it is better to test BooleanSimplification like original, but BooleanSimplificationSuite's optimizer mixes SimplifyConditionals and BooleanSimplification.

Yeah. What is the recommended way for adding test suite for optimizers? should each test suite only use the corresponding optimizer rule, or it's OK to also add those relevant ones?

Usually we only test corresponding rule in each test suite. But some suites use multiple rules like this one. For this one, although we can still only test BooleanSimplification if we add another optimizer, but seems too much. Maybe currently it's good enough. Maybe add a comment.

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

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

dbtsai · 2020-08-30T07:34:18Z

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

+    }
+
+    // check evaluation also
+    Seq(TrueLiteral, FalseLiteral).foreach { p =>


should we use the same name in the foreach?

dbtsai · 2020-08-30T07:35:18Z

LGTM except couple minor comments. +@dongjoon-hyun for another round of review before we merge it. Thanks.

SparkQA · 2020-08-30T12:54:46Z

Test build #128040 has finished for PR 29567 at commit 60e0e92.

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

viirya · 2020-08-30T16:58:46Z

Looks okay. Leave to @dongjoon-hyun for another check.

dongjoon-hyun · 2020-08-30T17:20:13Z

Thank you, @sunchao , @dbtsai , @viirya .

cc @gatorsmile since he expressed his concerns at that time.

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

dbtsai · 2020-08-31T03:03:08Z

cc @gatorsmile since he expressed his concerns at that time.

I think the concern @gatorsmile had was in #21850, converting CaseWhen to If, we will not get too much performance gain. However, this PR is trying to convert If to And or Or when possible, so we are able to pushdown more into data source. With this merged, we could revisit #21850 since it opens up the possibility that when we have one branch in CaseWhen, we can potentially optimize it to And or Or.

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

SparkQA · 2020-08-31T13:27:23Z

Test build #128086 has finished for PR 29567 at commit 2b90c19.

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

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

dbtsai · 2020-08-31T20:59:53Z

Merged into master. Thanks!

SparkQA · 2020-09-01T01:15:57Z

Test build #128118 has finished for PR 29567 at commit 71bd033.

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

cloud-fan · 2020-09-01T02:53:47Z

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

@@ -463,6 +463,8 @@ object SimplifyConditionals extends Rule[LogicalPlan] with PredicateHelper {
      case If(Literal(null, _), _, falseValue) => falseValue
      case If(cond, trueValue, falseValue)
        if cond.deterministic && trueValue.semanticEquals(falseValue) => trueValue
+      case If(cond, l @ Literal(null, _), FalseLiteral) if !cond.nullable => And(cond, l)


what about the other way around? e.g. case If(cond, FalseLiteral, l @ Literal(null, _))

ah it's covered in the followup #29603

### What changes were proposed in this pull request? This is a follow-up on SPARK-32721 and PR #29567. In the previous PR we missed two more cases that can be optimized: ``` if(p, false, null) ==> and(not(p), null) if(p, true, null) ==> or(p, null) ``` ### Why are the changes needed? By transforming if to boolean conjunctions or disjunctions, we can enable more filter pushdown to datasources. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added unit tests. Closes #29603 from sunchao/SPARK-32721-2. Authored-by: Chao Sun <sunchao@apache.org> Signed-off-by: DB Tsai <d_tsai@apple.com>

### What changes were proposed in this pull request? The following if clause: ```sql if(p, null, false) ``` can be simplified to: ```sql and(p, null) ``` Similarly, the clause: ```sql if(p, null, true) ``` can be simplified to ```sql or(not(p), null) ``` iff the predicate `p` is non-nullable, i.e., can be evaluated to either true or false, but not null. ### Why are the changes needed? Converting if to or/and clauses can better push filters down. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit tests. Closes apache#29567 from sunchao/SPARK-32721. Authored-by: Chao Sun <sunchao@apache.org> Signed-off-by: DB Tsai <d_tsai@apple.com>

### What changes were proposed in this pull request? This is a follow-up on SPARK-32721 and PR apache#29567. In the previous PR we missed two more cases that can be optimized: ``` if(p, false, null) ==> and(not(p), null) if(p, true, null) ==> or(p, null) ``` ### Why are the changes needed? By transforming if to boolean conjunctions or disjunctions, we can enable more filter pushdown to datasources. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Added unit tests. Closes apache#29603 from sunchao/SPARK-32721-2. Authored-by: Chao Sun <sunchao@apache.org> Signed-off-by: DB Tsai <d_tsai@apple.com>

SPARK-32721

f9e87f0

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

tanelk mentioned this pull request Aug 28, 2020

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

Closed

HyukjinKwon reviewed Aug 28, 2020

View reviewed changes

Fix bugs and merge into SimplifyConditionals

e4d929c

Fix lint

bb7c38c

sunchao force-pushed the SPARK-32721 branch from 81bcf2f to bb7c38c Compare August 28, 2020 17:56

Fix test failure

cc66198

sunchao changed the title ~~[WIP][SPARK-32721][SQL] Simplify if clauses with null and boolean~~ [SPARK-32721][SQL] Simplify if clauses with null and boolean Aug 29, 2020

viirya reviewed Aug 29, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala Outdated Show resolved Hide resolved

viirya reviewed Aug 30, 2020

View reviewed changes

dbtsai reviewed Aug 30, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala Outdated Show resolved Hide resolved

dbtsai reviewed Aug 30, 2020

View reviewed changes

...talyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalSuite.scala Outdated Show resolved Hide resolved

dbtsai reviewed Aug 30, 2020

View reviewed changes

Address comments

60e0e92

viirya approved these changes Aug 30, 2020

View reviewed changes

dongjoon-hyun reviewed Aug 30, 2020

View reviewed changes

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

dbtsai reviewed Aug 31, 2020

View reviewed changes

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

dbtsai approved these changes Aug 31, 2020

View reviewed changes

Test if evaluation with nullable condition

2b90c19

dbtsai reviewed Aug 31, 2020

View reviewed changes

...talyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/SimplifyConditionalSuite.scala Outdated Show resolved Hide resolved

viirya mentioned this pull request Aug 31, 2020

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

Closed

Fix indentation

71bd033

dbtsai closed this in 1453a09 Aug 31, 2020

sunchao mentioned this pull request Sep 1, 2020

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

Closed

cloud-fan reviewed Sep 1, 2020

View reviewed changes

		testRelationWithData.where(If('e.isNull, Literal.create(null, BooleanType), false)).analyze)
		testRelationWithData.where(And(Literal(null, BooleanType), 'e.isNull)).analyze)

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

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

Uh oh!

Conversation

sunchao commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Aug 28, 2020

Uh oh!

tanelk commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Aug 28, 2020

Uh oh!

sunchao commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanelk commented Aug 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 28, 2020

Uh oh!

SparkQA commented Aug 28, 2020

Uh oh!

SparkQA commented Aug 29, 2020

Uh oh!

sunchao commented Aug 29, 2020

Uh oh!

dbtsai commented Aug 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbtsai commented Aug 30, 2020

Uh oh!

SparkQA commented Aug 30, 2020

Uh oh!

viirya commented Aug 30, 2020

Uh oh!

dongjoon-hyun commented Aug 30, 2020

Uh oh!

Uh oh!

dbtsai commented Aug 31, 2020

Uh oh!

Uh oh!

SparkQA commented Aug 31, 2020

Uh oh!

Uh oh!

dbtsai commented Aug 31, 2020

Uh oh!

SparkQA commented Sep 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sunchao commented Aug 28, 2020 •

edited

Loading

tanelk commented Aug 28, 2020 •

edited

Loading

sunchao commented Aug 28, 2020 •

edited

Loading

dbtsai commented Aug 29, 2020 •

edited

Loading