[SPARK-33861][SQL] Simplify conditional in predicate #30865

wangyum · 2020-12-20T15:58:30Z

What changes were proposed in this pull request?

This pr simplify conditional in predicate, after this change we can push down the filter to datasource:

Expression	After simplify
IF(cond, trueVal, false)	AND(cond, trueVal)
IF(cond, trueVal, true)	OR(NOT(cond), trueVal)
IF(cond, false, falseVal)	AND(NOT(cond), elseVal)
IF(cond, true, falseVal)	OR(cond, elseVal)
CASE WHEN cond THEN trueVal ELSE false END	AND(cond, trueVal)
CASE WHEN cond THEN trueVal END	AND(cond, trueVal)
CASE WHEN cond THEN trueVal ELSE null END	AND(cond, trueVal)
CASE WHEN cond THEN trueVal ELSE true END	OR(NOT(cond), trueVal)
CASE WHEN cond THEN false ELSE elseVal END	AND(NOT(cond), elseVal)
CASE WHEN cond THEN false END	false
CASE WHEN cond THEN true ELSE elseVal END	OR(cond, elseVal)
CASE WHEN cond THEN true END	cond

Why are the changes needed?

Improve query performance.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test.

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

wangyum · 2020-12-21T02:47:25Z

retest this please.

SparkQA · 2020-12-21T04:08:55Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37724/

SparkQA · 2020-12-21T04:36:45Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37724/

SparkQA · 2020-12-21T07:40:11Z

Test build #133125 has finished for PR 30865 at commit f786f18.

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

SparkQA · 2020-12-22T18:03:16Z

Test build #133231 has finished for PR 30865 at commit 448faf0.

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

SparkQA · 2020-12-23T07:24:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37864/

SparkQA · 2020-12-23T07:55:07Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37864/

SparkQA · 2020-12-23T11:12:49Z

Test build #133270 has finished for PR 30865 at commit c90227a.

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

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

cloud-fan · 2020-12-23T11:53:24Z

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

+    case CaseWhen(Seq((cond, trueValue)), Some(TrueLiteral)) =>
+      Or(Not(cond), trueValue)
+    case CaseWhen(Seq((cond, FalseLiteral)), elseValue) =>
+      And(Not(cond), elseValue.getOrElse(Literal(null, BooleanType)))


elseValue.getOrElse(FalseLiteral)

cloud-fan · 2020-12-23T11:53:33Z

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

+    case CaseWhen(Seq((cond, FalseLiteral)), elseValue) =>
+      And(Not(cond), elseValue.getOrElse(Literal(null, BooleanType)))
+    case CaseWhen(Seq((cond, TrueLiteral)), elseValue) =>
+      Or(cond, elseValue.getOrElse(Literal(null, BooleanType)))


cloud-fan · 2020-12-23T11:54:06Z

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

+      And(Not(cond), elseValue.getOrElse(Literal(null, BooleanType)))
+    case CaseWhen(Seq((cond, TrueLiteral)), elseValue) =>
+      Or(cond, elseValue.getOrElse(Literal(null, BooleanType)))
+    case e if e.dataType == BooleanType => e


We can add an assert. The analyzer should guarantee it.

I mean something like

case e => assert(e.dataType != BooleanType, ...) e

It seems the added assert can never be triggered.
I think @cloud-fan meant also to remove the case e if e.dataType == BooleanType => e

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

SparkQA · 2020-12-24T01:37:00Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37919/

SparkQA · 2020-12-24T02:06:08Z

Test build #133327 has finished for PR 30865 at commit cccbaf1.

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

SparkQA · 2020-12-24T02:14:54Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37919/

SparkQA · 2020-12-24T04:27:59Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37927/

SparkQA · 2020-12-24T04:57:50Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37927/

SparkQA · 2020-12-24T05:08:54Z

Test build #133336 has finished for PR 30865 at commit 8bd9ef9.

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

cloud-fan · 2020-12-24T05:28:29Z

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

+import org.apache.spark.sql.types.BooleanType
+
+/**
+ * A rule that converting conditional expressions to predicate expressions, if possible, in the


that converting -> that converts

SparkQA · 2020-12-24T06:30:25Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37932/

SparkQA · 2020-12-24T07:05:04Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37932/

cloud-fan · 2020-12-24T08:10:27Z

thanks, merging to master!

SparkQA · 2020-12-24T09:59:34Z

Test build #133341 has finished for PR 30865 at commit 878beb0.

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

dongjoon-hyun · 2020-12-24T22:16:11Z

Thank you, @wangyum and @cloud-fan .

cloud-fan · 2021-01-06T06:01:30Z

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

+      And(cond, trueValue)
+    case CaseWhen(Seq((cond, trueValue)), Some(TrueLiteral)) =>
+      Or(Not(cond), trueValue)
+    case CaseWhen(Seq((_, FalseLiteral)), Some(FalseLiteral) | None) =>


This skips evaluating the condition, and we should make sure the condition is deterministic. @wangyum can you fix it?

OK, will fix it later.

and also it matters if cond throws an exception. Should probably think about NoThrow too.

Do we need to make these condition deterministic?

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

Lines 295 to 298 in 26d8df3

case FalseLiteral And _ => FalseLiteral

case _ And FalseLiteral => FalseLiteral

case TrueLiteral Or _ => TrueLiteral

case _ Or TrueLiteral => TrueLiteral

bart-samwel · 2021-01-06T13:01:05Z

I'm worried about this change. In ANSI dialect, conditions can easily throw exceptions, and this change could cause more conditions to be evaluated. E.g. IF(a, b, FALSE) turns into a AND b. But what if b can throw an error? Multiple things can happen:

If b can be pushed down but a cannot, then the shortcut evaluation behavior of AND is lost, and b is evaluated for values where a is not true. That is a semantic regression, because IF can be used to guard against such errors. An example case would be:

SELECT ...
FROM stores s JOIN store_revenues r ON s.id = r.store_id
WHERE IF(s.has_customers, r.revenue / r.num_customers > 0, FALSE)

In this case, the division filter can be pushed down into the scan of store_revenues, but it will cause division by zero.

Another case is where we are able to push down both a and b, and somehow the conjunctions get reordered in the process. E.g. IF(r.num_customers > 0, r.revenue / r.num_customers > 0, FALSE) could turn into r.revenue / r.num_customers > 0 AND r.num_customers > 0, doing the division by zero first. This reordering is not possible with IF, because that expression semantically enforces the ordering.

cloud-fan · 2021-01-06T13:34:42Z

Yea, I think we need to take care of the "may fail" property when optimizing expressions, as well as the "deterministic" property. Many optimizer rules need to be revisited.

… consider deterministic ### What changes were proposed in this pull request? This pr address #30865 (review) to fix simplify conditional in predicate should consider deterministic. ### Why are the changes needed? Fix bug. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit test. Closes #31067 from wangyum/SPARK-33861-2. Authored-by: Yuming Wang <yumwang@ebay.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

shardulm94 · 2022-08-30T00:00:33Z

@cloud-fan Was there any followup work done to address the issue pointed out by @bart-samwel where some expressions may fail is evaluated without the condition being evaluated first? Seems like converting IF to AND is not safe.

cloud-fan · 2022-08-30T01:42:50Z

conditional expressions is the only way people can control side effects. e.g. IF(cond, trueExpr, falseExpr) guarantees that trueExpr will only be evaluated if condition is true. This optimization breaks it and the benefits do not seem significant.

@wangyum shall we revert this optimizer rule? cc @viirya @rednaxelafx @sigmod

wangyum · 2022-08-30T13:18:32Z

OK. revert pr: #37729

Simplify conditional in predicate

f786f18

wangyum marked this pull request as draft December 20, 2020 15:58

github-actions bot added the SQL label Dec 20, 2020

dongjoon-hyun reviewed Dec 21, 2020

View reviewed changes

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

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'upstream/master' into SPARK-33861

45fd7a5

This comment has been minimized.

Sign in to view

fix

448faf0

wangyum added 2 commits December 23, 2020 14:32

Merge remote-tracking branch 'upstream/master' into SPARK-33861

94551ea

fix

c90227a

wangyum changed the title ~~[WIP][SPARK-33861][SQL] Simplify conditional in predicate~~ [SPARK-33861][SQL] Simplify conditional in predicate Dec 23, 2020

wangyum marked this pull request as ready for review December 23, 2020 06:57

wangyum requested review from cloud-fan, dbtsai, dongjoon-hyun, HyukjinKwon and maropu December 23, 2020 09:10

cloud-fan reviewed Dec 23, 2020

View reviewed changes

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

cloud-fan reviewed Dec 23, 2020

View reviewed changes

fix

cccbaf1

wangyum commented Dec 24, 2020

View reviewed changes

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

wangyum commented Dec 24, 2020

View reviewed changes

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

further optimize

8bd9ef9

cloud-fan reviewed Dec 24, 2020

View reviewed changes

cloud-fan approved these changes Dec 24, 2020

View reviewed changes

fix

878beb0

cloud-fan closed this in 32d4a2b Dec 24, 2020

wangyum deleted the SPARK-33861 branch December 24, 2020 08:12

cloud-fan reviewed Jan 6, 2021

View reviewed changes

wangyum mentioned this pull request Jan 6, 2021

[SPARK-33861][SQL][FOLLOWUP] Simplify conditional in predicate should consider deterministic #31067

Closed

	case FalseLiteral And _ => FalseLiteral
	case _ And FalseLiteral => FalseLiteral
	case TrueLiteral Or _ => TrueLiteral
	case _ Or TrueLiteral => TrueLiteral

[SPARK-33861][SQL] Simplify conditional in predicate #30865

[SPARK-33861][SQL] Simplify conditional in predicate #30865

Uh oh!

Conversation

wangyum commented Dec 20, 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!

Uh oh!

wangyum commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

SparkQA commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 23, 2020

Uh oh!

SparkQA commented Dec 23, 2020

Uh oh!

SparkQA commented Dec 23, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

cloud-fan commented Dec 24, 2020

Uh oh!

SparkQA commented Dec 24, 2020

Uh oh!

dongjoon-hyun commented Dec 24, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bart-samwel commented Jan 6, 2021

Uh oh!

wangyum commented Dec 20, 2020 •

edited

Loading