[SPARK-34222][SQL] Enhance boolean simplification rule #31318

Swinky · 2021-01-25T10:06:54Z

What changes were proposed in this pull request?

Enhance boolean simplification rule by handling following scenarios:
(((a && b) && a && (a && c))) => a && b && c)
(((a || b) || a || (a || c))) => a || b || c

Why are the changes needed?

Minor improvement

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added UTs

Swinky · 2021-02-17T10:35:36Z

@cloud-fan,@maropu,
could you please have a look at this. thanks !

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

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

maropu · 2021-02-18T00:45:35Z

ok to test

maropu · 2021-02-18T00:45:49Z

cc: @wangyum

SparkQA · 2021-02-18T02:06:30Z

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

SparkQA · 2021-02-18T03:43:54Z

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

SparkQA · 2021-02-18T04:33:55Z

Test build #135209 has finished for PR 31318 at commit 8b9ba7f.

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

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

SparkQA · 2021-02-19T18:56:22Z

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

SparkQA · 2021-02-19T19:00:47Z

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

SparkQA · 2021-02-19T22:59:18Z

Test build #135275 has finished for PR 31318 at commit 3b188a2.

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

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

cloud-fan · 2021-02-22T04:37:37Z

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

+            and
+          } else {
+            // (((a && b) && a && (a && c))) => a && b && c
+            distinct.reduce(And)


one concern is, if there are many conjunctive predicates, here we may build a left-deep tree while the original one is balanced. Can we add a util function in PredicateHelper to build a balanced And/Or tree?

cc @gengliangwang

@cloud-fan yes that would be helpful, especially in filter pushdown of data sources.

thanks @cloud-fan, good observation. I have added required Utils to address this.

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

SparkQA · 2021-03-01T06:35:36Z

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

SparkQA · 2021-03-01T06:44:30Z

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

SparkQA · 2021-03-01T10:19:26Z

Test build #135580 has finished for PR 31318 at commit 7ce6731.

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

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

SparkQA · 2021-03-01T19:26:49Z

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

SparkQA · 2021-03-01T19:35:12Z

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

SparkQA · 2021-03-01T23:33:04Z

Test build #135606 has finished for PR 31318 at commit 2c0d72c.

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

maropu

Left the minor comments and it looks fine. Could you take another look? @cloud-fan @gengliangwang @c21

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

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

SparkQA · 2021-03-02T04:13:23Z

Test build #135624 has started for PR 31318 at commit 5988c8f.

SparkQA · 2021-03-02T04:31:28Z

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

SparkQA · 2021-03-02T04:39:51Z

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

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

gengliangwang

Thanks for the work, @Swinky !

c21

LGTM with only one minor comment.

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

SparkQA · 2021-03-02T18:14:04Z

Test build #135669 has started for PR 31318 at commit 699c1f4.

SparkQA · 2021-03-02T18:51:53Z

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

SparkQA · 2021-03-02T19:27:04Z

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

cloud-fan · 2021-03-03T05:25:26Z

thanks, merging to master!

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

maropu · 2021-03-08T01:09:43Z

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

+            or
+          } else {
+            // (a || b) || a || (a || c) => a || b || c
+            buildBalancedPredicate(distinct.toSeq, Or)


Is it okay to call distinct.toSeq here? The current implementation of ExpressionSet keeps the original order of expressions, but I'm not sure that its design guarantees the order.

@maropu - ExpressionSet implements Iterable.iterator() method, so .toSeq will return the expressions in original order, right? Do you think we need to add more documentation on ExpressionSet to harden this assumption?

Do you think we need to add more documentation on ExpressionSet to harden this assumption?

Yea, documenting it explicitly looks better to me.

Swinky added 2 commits January 25, 2021 01:15

enhance boolean simplification rule

088822e

testname

8b9ba7f

github-actions bot added the SQL label Jan 25, 2021

maropu reviewed Feb 18, 2021

View reviewed changes

cloud-fan reviewed Feb 18, 2021

View reviewed changes

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

address PR comments

3b188a2

cloud-fan reviewed Feb 22, 2021

View reviewed changes

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

cloud-fan reviewed Feb 22, 2021

View reviewed changes

c21 reviewed Feb 22, 2021

View reviewed changes

Swinky added 2 commits February 22, 2021 23:08

address formatting comments

a7569aa

address PR comments

7ce6731

maropu reviewed Mar 1, 2021

View reviewed changes

address comments

2c0d72c

maropu approved these changes Mar 2, 2021

View reviewed changes

formatting

5988c8f

gengliangwang reviewed Mar 2, 2021

View reviewed changes

...lyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala Outdated Show resolved Hide resolved

gengliangwang reviewed Mar 2, 2021

View reviewed changes

...lyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/BooleanSimplificationSuite.scala Outdated Show resolved Hide resolved

gengliangwang approved these changes Mar 2, 2021

View reviewed changes

c21 reviewed Mar 2, 2021

View reviewed changes

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

address comments

699c1f4

Swinky mentioned this pull request Mar 2, 2021

[SPARK-34598][SQL] RewritePredicateSubquery Rule must not update Filters without subqueries #31712

Closed

cloud-fan approved these changes Mar 3, 2021

View reviewed changes

cloud-fan closed this in 229d2e0 Mar 3, 2021

HyukjinKwon reviewed Mar 7, 2021

View reviewed changes

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

maropu reviewed Mar 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-34222][SQL] Enhance boolean simplification rule #31318

[SPARK-34222][SQL] Enhance boolean simplification rule #31318

Swinky commented Jan 25, 2021

Swinky commented Feb 17, 2021 •

edited

Loading

maropu commented Feb 18, 2021

maropu commented Feb 18, 2021

SparkQA commented Feb 18, 2021

SparkQA commented Feb 18, 2021

SparkQA commented Feb 18, 2021

SparkQA commented Feb 19, 2021

SparkQA commented Feb 19, 2021

SparkQA commented Feb 19, 2021

cloud-fan Feb 22, 2021

gengliangwang Feb 22, 2021

Swinky Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

maropu left a comment

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

gengliangwang left a comment

c21 left a comment

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

cloud-fan commented Mar 3, 2021

maropu Mar 8, 2021

c21 Mar 11, 2021

maropu Mar 12, 2021

[SPARK-34222][SQL] Enhance boolean simplification rule #31318

[SPARK-34222][SQL] Enhance boolean simplification rule #31318

Conversation

Swinky commented Jan 25, 2021

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?

Swinky commented Feb 17, 2021 • edited Loading

maropu commented Feb 18, 2021

maropu commented Feb 18, 2021

SparkQA commented Feb 18, 2021

SparkQA commented Feb 18, 2021

SparkQA commented Feb 18, 2021

SparkQA commented Feb 19, 2021

SparkQA commented Feb 19, 2021

SparkQA commented Feb 19, 2021

cloud-fan Feb 22, 2021

Choose a reason for hiding this comment

gengliangwang Feb 22, 2021

Choose a reason for hiding this comment

Swinky Mar 1, 2021

Choose a reason for hiding this comment

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

SparkQA commented Mar 1, 2021

maropu left a comment

Choose a reason for hiding this comment

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

gengliangwang left a comment

Choose a reason for hiding this comment

c21 left a comment

Choose a reason for hiding this comment

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

SparkQA commented Mar 2, 2021

cloud-fan commented Mar 3, 2021

maropu Mar 8, 2021

Choose a reason for hiding this comment

c21 Mar 11, 2021

Choose a reason for hiding this comment

maropu Mar 12, 2021

Choose a reason for hiding this comment

Swinky commented Feb 17, 2021 •

edited

Loading