[SPARK-42500][SQL] ConstantPropagation supports more cases by wangyum · Pull Request #40093 · apache/spark

wangyum · 2023-02-20T11:10:51Z

What changes were proposed in this pull request?

This PR enhances ConstantPropagation to support more cases.

Propagated through other binary comparisons.
Propagated across equality comparisons. This can be further optimized to false.

For example:

a =1and b > a +2==> a =1 && b >3
a =1and a =2==>2=1and1=2==> false

Why are the changes needed?

Improve query performance. Denodo also has a similar optimization. For example:

CREATETABLEt1(a int, b int) using parquet;
CREATETABLEt2(x int, y int) using parquet;
CREATE TEMP VIEW v1 ASSELECT*FROM t1 JOIN t2 WHERE a = x AND a =0UNION ALLSELECT*FROM t1 JOIN t2 WHERE a = x AND (a IS NULLOR a <>0);
SELECT*FROM v1 WHERE x >1;

Before this PR:

== Optimized Logical Plan ==
Union false, false
:- Project [a#0 AS a#12, b#1 AS b#13, x#2 AS x#14, y#3 AS y#15]
: +- Join Inner
: :- Filter (isnotnull(a#0) AND (a#0 = 0))
: : +- Relation spark_catalog.default.t1[a#0,b#1] parquet
: +- Filter (isnotnull(x#2) AND ((0 = x#2) AND (x#2 > 1)))
: +- Relation spark_catalog.default.t2[x#2,y#3] parquet
+- Join Inner, (a#16 = x#18)
:- Filter ((isnull(a#16) OR NOT (a#16 = 0)) AND ((a#16 > 1) AND isnotnull(a#16)))
: +- Relation spark_catalog.default.t1[a#16,b#17] parquet
+- Filter ((isnotnull(x#18) AND (x#18 > 1)) AND (isnull(x#18) OR NOT (x#18 = 0)))
+- Relation spark_catalog.default.t2[x#18,y#19] parquet

After this PR:

== Optimized Logical Plan ==
Join Inner, (a#16 = x#18)
:- Filter ((isnull(a#16) OR NOT (a#16 = 0)) AND ((a#16 > 1) AND isnotnull(a#16)))
: +- Relation spark_catalog.default.t1[a#16,b#17] parquet
+- Filter ((isnotnull(x#18) AND (x#18 > 1)) AND (isnull(x#18) OR NOT (x#18 = 0)))
+- Relation spark_catalog.default.t2[x#18,y#19] parquet

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Unit test.

wangyum · 2023-02-21T01:55:41Z

 condition transform {
- case e @ EqualTo(_, _) if !predicates.contains(e) => replaceConstants0(e)
- case e @ EqualNullSafe(_, _) if !predicates.contains(e) => replaceConstants0(e)
+ case b: BinaryComparison =>


To support other binary comparisons. For example: >, <.

Can we look at the commit history and understand why constant propagation only supports equality predicates before?

The initial commit did not support it. It seems other binary comparisons were not considered at the time:
#17993
Later we tried to enhance it. It seems it is not easy because there are too many changes:
#24553

cc @peter-toth

I think we can replace constants in other expressions too, #24553 does the same (and more).

I'm not sure I get the issue. If a complex expression is equal to a constant why we can't use that constant where the complex expression appears? I don't think it matters if some parts of the complex expression can also be replaced to another constat. The replace happens using transformDown so larger complex expressions are replaced to constants first. E.g. a = 1 AND (a + b) = 1 AND (a + b) > 2 -> a = 1 AND (1 + b) = 1 AND 1 > 2 -> false makes sense to me.
Yes we can. The reason why I didn't do those kind of shortcuts in ConstantPropagation in my PR is because there are other, similar cases where it isn't that easy to do a shortcut (or simplification has been handled in other rules so why doing it here as well) . E.g. a = 1 AND a > 2 -> a = 1 AND 1 > 2 -> false where the 2nd step (1 > 2 -> false) could also be handled in ConstantPropagation but those foldable expressions are already handled in ConstantFolding and then in BooleanSimplification when contant propagation has been done.

Let me open a PR tomorrow.

I've opened #40268.
I didn't want to overcomplicate the change so took only a few parts from #24553. But if you think 1. or 2. or any other feature from the original PR is also needed I can add it to the PR.

@cloud-fan This is why conflicting equality predicates are not supported: #17993 (comment)
It is correct if it only optimize on the filter condition.

Sidenote: In my old PR (https://github.com/apache/spark/pull/24553/files#diff-d43484d56a4d9991066b5c00d12ec2465c75131e055fc02ee7fb6dfd45b5006fR76-R79) I proposed #27309 to detect equijoins better and to allow constant propagation in Joins too.

wangyum · 2023-02-21T01:59:38Z

+ if (!allPredicates.contains(b)) {
+ replaceConstants0(b, allConstantsMap)
+ } else {
+ val excludedEqualityPredicates = equalityPredicates.filterNot(_._2.semanticEquals(b))


Exclude current binary comparison to support the following case:
a = 1 and a = 2 ==> 2 = 1 and 1 = 2

this doesn't look like constant propagation but a different optimization to simplify predicates:
a = 1 and a = 2 ==> false
a > 1 and a > 2 ==> a > 2

This PR does not support a > 1 and a > 2 ==> a > 2. It must contain an equality predicate.

Can we exclude it first? We should add a new optimizer rule later to do more advanced predicate simplification.

Do you mean only support a = 1 and a = 2 ==> 2 = 1 and 1 = 2 ==> false, and do not support a = 1 and b > a + 2 ==> a = 1 && b > 3?

nvm, the rule name is ConstantPropagation, so both optimizations fit it.

wangyum · 2023-02-21T02:42:44Z

cc @cloud-fan

wangyum · 2023-02-26T02:37:12Z

TiDB also supports these optimizations:

github-actions · 2023-07-08T00:22:12Z

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!

github-actionsBot added the SQL label Feb 20, 2023

SPARK-42500: ConstantPropagation support more case
88c6d0b

wangyum force-pushed the SPARK-42500 branch from 242c844 to 88c6d0bCompare February 21, 2023 00:12

wangyum commented Feb 21, 2023
View reviewed changes

peter-toth mentioned this pull request Mar 3, 2023
[SPARK-42500][SQL] ConstantPropagation support more cases #40268
Closed

github-actionsBot added the Stale label Jul 8, 2023

github-actionsBot closed this Jul 9, 2023

Uh oh!

Conversation

wangyum commented Feb 20, 2023• 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!

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!

peter-tothMar 1, 2023• 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangyum commented Feb 21, 2023

Uh oh!

wangyum commented Feb 26, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Jul 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wangyum commented Feb 20, 2023•
edited
Loading

peter-tothMar 1, 2023•
edited
Loading

wangyum commented Feb 26, 2023•
edited
Loading