[SPARK-27604][SQL] Add filter reduction by peter-toth · Pull Request #24495 · apache/spark

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

- * 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.

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

- * 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

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

+ 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

+ * - 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.

Change-Id: I8ba31ae5043184d53662a4cedfe011a36d63759f

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

+ 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

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

+ 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

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

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 reductionMay 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

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

remove constant propagation changes
693cdbb
Change-Id: I8ba31ae5043184d53662a4cedfe011a36d63759f

dongjoon-hyun reviewed May 8, 2019
View reviewed changes

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

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-actionsBot added the Stale label Apr 6, 2020

github-actionsBot closed this Apr 7, 2020

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-hyunApr 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-tothMay 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-tothMay 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-tothMay 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-actionsBot commented Apr 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

peter-toth commented Apr 30, 2019•
edited
Loading

peter-toth commented Apr 30, 2019•
edited
Loading

dongjoon-hyunApr 30, 2019•
edited
Loading

peter-tothMay 6, 2019•
edited
Loading

peter-tothMay 8, 2019•
edited
Loading

dongjoon-hyun commented May 7, 2019•
edited
Loading

peter-tothMay 9, 2019•
edited
Loading