[SPARK-27604][SQL] Add filter reduction #24495

peter-toth · 2019-04-30T10:30:23Z

What changes were proposed in this pull request?

This PR adds a new FilterReduction rule that can reduce BinaryComparison filters like these:

SELECT * FROM table WHERE i <= 5 AND i = 5        => ... WHERE i = 5
SELECT * FROM table WHERE i < j AND ... AND i > j => ... WHERE false

How was this patch tested?

Existing and new UTs.

…tion

peter-toth · 2019-04-30T10:36:59Z

I filed https://issues.apache.org/jira/browse/SPARK-27604 to cover some improvements regarding constraints. This PR adds some improvement although there is room for more.
I'm interested in feedbacks about this PR and also in suggestions how we could take this further.

cc. @gengliangwang @cloud-fan @mgaido91 @viirya @hvanhovell @gatorsmile @dongjoon-hyun as I saw you worked on similar topics before.

Change-Id: I8031a37f380d5fe5fff37488460b790dd65c0161

Change-Id: I79ec5286b4762d98abaea638f8a9e5e9f7f753d0

dongjoon-hyun · 2019-04-30T15:44:17Z

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

- *   SELECT * FROM table WHERE i = 5 AND j = i + 3
- *   ==>  SELECT * FROM table WHERE i = 5 AND j = 8
+ *   SELECT * FROM table WHERE i = 5 AND j = i + 3     => SELECT * FROM table WHERE i = 5 AND j = 8
+ *   SELECT * FROM table WHERE i <= 5 AND i = 5        => SELECT * FROM table WHERE i = 5


Sorry, @peter-toth . I don't think this aims the equal goal. Could you make a separate rule instead?

Filter reduction should be a separate rule.

Thank you all for the feedback. The 2 rules seemed similar and easy to combine to me, but I will not mix them then.

I separated the 2 rules. I added some improvement to the ConstantPropagation and created a new FilterReduction.

I've moved all new filter reduction related logic to FilterReduction and extracted constant propagation enhancements to a separate PR.

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

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

mgaido91 · 2019-05-01T09:25:44Z

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

- *   SELECT * FROM table WHERE i = 5 AND j = i + 3
- *   ==>  SELECT * FROM table WHERE i = 5 AND j = 8
+ *   SELECT * FROM table WHERE i = 5 AND j = i + 3     => SELECT * FROM table WHERE i = 5 AND j = 8
+ *   SELECT * FROM table WHERE i <= 5 AND i = 5        => SELECT * FROM table WHERE i = 5


Change-Id: Ia68242ba5067e84143e35a1126a49017c5a203f6

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

...talyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/QueryPlanConstraints.scala

mgaido91 · 2019-05-07T08:32:26Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/PlanTest.scala

+        val newCondition =
+          splitConjunctivePredicates(condition)
+            .map(rewriteEqual)
+            .sortBy(p => scala.util.hashing.MurmurHash3.seqHash(Seq(p.getClass, p)))


why do we need to change this?

I added the class of expression to the sorting order here and in rewriteEqual() as hashCode() of TreeNode doesn't take the class of the expression into account and
comparePlans(testRelation.where('a < 2 && 'a === 2).analyze, testRelation.where('a === 2 && 'a < 2).analyze) would fail.

Change-Id: Ice97dbafb67bf6cf84107b9064f42be991cbd9f8

dongjoon-hyun · 2019-05-07T20:45:58Z

Hi, @peter-toth . Thank you for splitting the rule.
Please split ConstantPropagation improvement into a separate PR. That doesn't depend on the new FilterReduction, right? That makes the review easier and faster.

peter-toth · 2019-05-08T07:09:26Z

Thanks @dongjoon-hyun, I extracted the improved ConstantPropagation rule to here: #24553

dongjoon-hyun · 2019-05-08T18:37:33Z

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

+ * - Using this mapping, replace occurrence of the expressions with the corresponding constant
+ *   values in the AND node.
 */
 object ConstantPropagation extends Rule[LogicalPlan] with PredicateHelper {


Since new PR is created, we had better remove this change from this PR.

Ok, reverted.
Please note that some of the new UTs will require the enhanced constant propagation to work as expected.

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

Change-Id: I8ba31ae5043184d53662a4cedfe011a36d63759f

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterReductionSuite.scala

dongjoon-hyun · 2019-05-08T20:37:37Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterReductionSuite.scala

+      Batch("AnalysisNodes", Once,
+        EliminateSubqueryAliases) ::
+        Batch("FilterReduction", FixedPoint(10),
+          ConstantPropagation,


Which test case requires this?

Like a <= 2 AND 2 <= a where I would expect a = 2 reduced result. FilterReduction can transforms it into a <= 2 AND 2 = a. Then ConstantPropagation (only the enhanced in #24553) can transform it into 2 <= 2 AND 2 = a ...

Change-Id: Ifef383125dd8827fdeea90e8101cb432de9e704e

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

mgaido91 · 2019-05-09T08:48:34Z

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

+        case c EqualNullSafe d if planEqual(a, d) => EqualTo(c, d)
+      }
+      case a EqualTo b => expression transformUp {
+        case c LessThan d if planEqual(b, d) && planEqual(a, c) => Literal.FalseLiteral


actually these too are wrong with nulls, as they should return null instead of false and the difference is significant (consider the case of a Not in front of it..)

Thanks! I think you are right and this is an issue at many other places too.

I think the solution could be checking if a, b, c and d not nullable and then we can use transform. Otherwise limiting a top-down way replacement until we have any of And, Or, CaseWhen or If nodes and can't go deeper if we hit something else. This is what ReplaceNullWithFalseInPredicate rule does.

Fixed the issue and added some new UTs.

HyukjinKwon · 2019-05-23T18:40:38Z

ok to test

SparkQA · 2019-05-23T20:04:40Z

Test build #105735 has finished for PR 24495 at commit 0972a29.

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

peter-toth · 2019-05-23T20:39:45Z

Thanks @HyukjinKwon for enabling the tests. Some UT failures are expected until #24553 is not merged.

cloud-fan · 2019-12-27T06:45:13Z

can we enrich the PR description with how it is implemented? e.g. the overall design, the algorithm that is used.

peter-toth · 2019-12-27T11:40:55Z

can we enrich the PR description with how it is implemented? e.g. the overall design, the algorithm that is used.

@cloud-fan, I'm happy to take this up again, but there is a prerequisite PR to this one which I would like to polish first: #24553

github-actions · 2020-04-06T00:09:13Z

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!

[SPARK-27604][SQL] Enhance constant propagation to constraint propaga…

dd09f51

…tion

peter-toth added 2 commits April 30, 2019 16:31

normalize only once

d1ae18c

Change-Id: I8031a37f380d5fe5fff37488460b790dd65c0161

fix null handling regarding EqualNullSafe

8aa8e36

Change-Id: I79ec5286b4762d98abaea638f8a9e5e9f7f753d0

dongjoon-hyun reviewed Apr 30, 2019

View reviewed changes

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

mgaido91 reviewed May 1, 2019

View reviewed changes

peter-toth added 2 commits May 6, 2019 13:35

fix review findings

3737801

revert unnecessary changes, minor fix

9ee3be3

Change-Id: Ia68242ba5067e84143e35a1126a49017c5a203f6

mgaido91 reviewed May 7, 2019

View reviewed changes

peter-toth added 2 commits May 7, 2019 13:14

revert accidentally removed logic, add more tests

169563b

Change-Id: Ice97dbafb67bf6cf84107b9064f42be991cbd9f8

use map of equality predicates in ConstantPropagation

3c982b6

peter-toth changed the title ~~[SPARK-27604][SQL] Enhance constant propagation to constraint propagation~~ [SPARK-27604][SQL] Add filter reduction May 8, 2019

peter-toth mentioned this pull request May 8, 2019

[SPARK-27604][SQL] Enhance constant propagation #24553

Closed

minor fix

b0786e1

dongjoon-hyun reviewed May 8, 2019

View reviewed changes

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

remove constant propagation changes

693cdbb

Change-Id: I8ba31ae5043184d53662a4cedfe011a36d63759f

dongjoon-hyun reviewed May 8, 2019

View reviewed changes

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

dongjoon-hyun reviewed May 8, 2019

View reviewed changes

fix review findings

9dc1422

Change-Id: Ifef383125dd8827fdeea90e8101cb432de9e704e

mgaido91 reviewed May 9, 2019

View reviewed changes

peter-toth added 2 commits May 10, 2019 15:09

fix null issues

699fe2f

plantest fix

0972a29

dongjoon-hyun added the SQL label Jun 14, 2019

github-actions bot added the Stale label Apr 6, 2020

github-actions bot closed this Apr 7, 2020

[SPARK-27604][SQL] Add filter reduction #24495

[SPARK-27604][SQL] Add filter reduction #24495

Uh oh!

Conversation

peter-toth commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

peter-toth commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

peter-toth May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-toth May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

dongjoon-hyun commented May 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peter-toth commented May 8, 2019

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-toth May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented May 23, 2019

Uh oh!

SparkQA commented May 23, 2019

Uh oh!

peter-toth commented May 23, 2019

Uh oh!

cloud-fan commented Dec 27, 2019

Uh oh!

peter-toth commented Dec 27, 2019

Uh oh!

github-actions bot commented Apr 6, 2020

Uh oh!

Reviewers

Assignees

peter-toth commented Apr 30, 2019 •

edited

Loading

peter-toth commented Apr 30, 2019 •

edited

Loading

dongjoon-hyun Apr 30, 2019 •

edited

Loading

peter-toth May 6, 2019 •

edited

Loading

peter-toth May 8, 2019 •

edited

Loading

dongjoon-hyun commented May 7, 2019 •

edited

Loading

peter-toth May 9, 2019 •

edited

Loading