[SPARK-30876][SQL] Optimizer fails to infer constraints within join #29170

navinvishy · 2020-07-20T23:49:47Z

What changes were proposed in this pull request?

For a 3-way join of the kind described below, the optimizer fails to infer the constraint a=1. This appears to be because of the interaction between the following rules: ColumnPruning, InferFiltersFromConstraints, and PredicatePushdown.

For the following SQL query:

create table t1(a int, b int, c int);
create table t2(a int, b int, c int);
create table t3(a int, b int, c int);
select count(*) from t1 join t2 join t3 on (t1.a = t2.b and t2.b = t3.c and t3.c = 1);

The optimized logical plan produced is:

== Optimized Logical Plan ==
Aggregate [count(1) AS count(1)#66L]
+- Project
   +- Join Inner, (b#61 = c#65)
      :- Project [b#61]
      :  +- Join Inner, (a#57 = b#61)
      :     :- Project [a#57]
      :     :  +- Filter isnotnull(a#57)
      :     :     +- HiveTableRelation `default`.`t1`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, [a#57, b#58, c#59], Statistics(sizeInBytes=8.0 EiB)
      :     +- Project [b#61]
      :        +- Filter (isnotnull(b#61) AND (b#61 = 1))
      :           +- HiveTableRelation `default`.`t2`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, [a#60, b#61, c#62], Statistics(sizeInBytes=8.0 EiB)
      +- Project [c#65]
         +- Filter (isnotnull(c#65) AND (c#65 = 1))
            +- HiveTableRelation `default`.`t3`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, [a#63, b#64, c#65], Statistics(sizeInBytes=8.0 EiB)

The constraint a=1 does not get inferred because after column pruning, the InferFiltersFromConstraints rule on the outer join operator only infers b=1, since the project operator around the join operator drops the constraint that refers to 'a'. Later, when PushdownPredicates runs again, the b=1 constraint gets pushed down to relation 'y'.

In order to resolve this, this patch proposes replacing the InferFiltersFromConstraints once batch with a batch of InferFiltersFromConstraints and PredicatePushdown to fixed point. With this change, the constraint b=1 will be inferred and pushed down first. The constraint a=1 can then be inferred for the inner join operator because b=1 is now available in order to infer it. Running PredicatePushdown again will push a=1 to its correct position.

Why are the changes needed?

Improves performance of optimization. Also this worked correctly earlier in 2.3.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added a unit test. The test should fail if InferFiltersFromConstraints is run only once.

What changes were proposed in this pull request? For a 3-way join of the kind described below, the optimizer fails to infer the constraint a=1. This is because of the interaction between the following rules: ColumnPruning, InferFiltersFromConstraints, and PredicatePushdown. For the following SQL query: ``` create table t1(a int, b int, c int); create table t2(a int, b int, c int); create table t3(a int, b int, c int); select count(*) from t1 join t2 join t3 on (t1.a = t2.b and t2.b = t3.c and t3.c = 1); ``` The optimized logical plan produced is: ``` == Optimized Logical Plan == Aggregate [count(1) AS count(1)#66L] +- Project +- Join Inner, (b#61 = c#65) :- Project [b#61] : +- Join Inner, (a#57 = b#61) : :- Project [a#57] : : +- Filter isnotnull(a#57) : : +- HiveTableRelation `default`.`t1`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, [a#57, b#58, c#59], Statistics(sizeInBytes=8.0 EiB) : +- Project [b#61] : +- Filter (isnotnull(b#61) AND (b#61 = 1)) : +- HiveTableRelation `default`.`t2`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, [a#60, b#61, c#62], Statistics(sizeInBytes=8.0 EiB) +- Project [c#65] +- Filter (isnotnull(c#65) AND (c#65 = 1)) +- HiveTableRelation `default`.`t3`, org.apache.hadoop.hive.serde2.lazy.LazySimpleSerDe, [a#63, b#64, c#65], Statistics(sizeInBytes=8.0 EiB) ``` The constraint gets dropped because after column pruning, the InferFiltersFromConstraints rule on the outer join operator only infers b=1, since the project operator drops the constraint that refers to 'a'. Later, when PushdownPredicates runs again, the b=1 constraint gets pushed down to relation 'y'. In order to resolve this, this patch proposes running InferFiltersFromConstraints and PredicatePushdown in a batch to fixed point, in place of running InferFiltersFromConstraints once in a batch. With this change, the constraint b=1 will be inferred and pushed down first. Then, the constraint a=1 will be inferred for the inner join operator since that can be inferred because b=1 is now available to infer it. Running PredicatePushdown again will push a=1 to its correct position. Why are the changes needed? Improves performance of optimization. Also this worked correctly earlier in 2.3. Does this PR introduce any user-facing change? No How was this patch tested? Added a unit test. The test should fail if InferFiltersFromConstraints is run only once.

maropu · 2020-07-21T06:20:35Z

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

@@ -116,7 +116,8 @@ abstract class Optimizer(catalogManager: CatalogManager)
        operatorOptimizationRuleSet.filterNot(_ == InferFiltersFromConstraints)
      Batch("Operator Optimization before Inferring Filters", fixedPoint,
        rulesWithoutInferFiltersFromConstraints: _*) ::
-      Batch("Infer Filters", Once,
+      Batch("Infer Filters", fixedPoint,
+        PushDownPredicates,
        InferFiltersFromConstraints) ::


Note: This rule was separated because #19149

maropu · 2020-07-21T06:20:42Z

ok to test

maropu · 2020-07-21T06:21:04Z

Thanks for the contribution, @navinvishy

SparkQA · 2020-07-21T07:05:02Z

Test build #126223 has finished for PR 29170 at commit 6f01ac9.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-21T07:40:55Z

retest this please

SparkQA · 2020-07-21T12:38:15Z

Test build #126233 has finished for PR 29170 at commit 6f01ac9.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-21T13:01:21Z

retest this please

SparkQA · 2020-07-21T17:48:27Z

Test build #126252 has finished for PR 29170 at commit 6f01ac9.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-22T00:03:28Z

retest this please

SparkQA · 2020-07-22T05:55:43Z

Test build #126285 has finished for PR 29170 at commit 6f01ac9.

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

navinvishy · 2020-07-22T16:07:33Z

Thanks for looking into this @maropu ! Do you know anything about this test failure? Looks unrelated to the change.

SparkQA · 2020-07-24T01:12:32Z

Test build #126448 has finished for PR 29170 at commit 51977bc.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-07-24T07:05:03Z

Test build #126450 has finished for PR 29170 at commit 2b3c82a.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-27T23:59:36Z

Thanks for looking into this @maropu ! Do you know anything about this test failure? Looks unrelated to the change.

Yea, the failure is not related to this.

maropu · 2020-07-27T23:59:47Z

retest this please

SparkQA · 2020-07-28T05:11:15Z

Test build #126675 has finished for PR 29170 at commit 2b3c82a.

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

navinvishy · 2020-08-01T02:03:19Z

@maropu wanted to circle back on this. Do you think this can be merged?

peter-toth · 2020-11-03T17:47:22Z

Hmm, I've run into this issue many times recently. Spark 2.3+ runs InferFiltersFromConstraints rule only once due to #19149, which causes performance regression from 2.2.
@navinvishy, @maropu, @cloud-fan, @gatorsmile do you think you can revisit this PR?

maropu · 2020-11-06T07:22:45Z

@navinvishy, @maropu, @cloud-fan, @gatorsmile do you think you can revisit this PR?

Revisiting this looks fine to me. Are you still here? @navinvishy

maropu · 2020-11-06T07:30:42Z

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

@@ -116,7 +116,8 @@ abstract class Optimizer(catalogManager: CatalogManager)
        operatorOptimizationRuleSet.filterNot(_ == InferFiltersFromConstraints)
      Batch("Operator Optimization before Inferring Filters", fixedPoint,
        rulesWithoutInferFiltersFromConstraints: _*) ::
-      Batch("Infer Filters", Once,
+      Batch("Infer Filters", fixedPoint,
+        PushDownPredicates,


My concern is that the change above might cause the issue described in #19149. Instead, (just a suggestion), we couldn't improve InferFiltersFromConstraints to cover the case you pointed out in the PR description?

Unfortunately, there is no UT in the commit to see what was the exact issue and why InferFiltersFromConstraints needs to be separated entirely from other rules.

If I get the issue described in https://issues.apache.org/jira/browse/SPARK-21652 right, if we combine InferFiltersFromConstraints with other rules into a batch then we can end up in an infinite loop in that batch. But this is only because InferFiltersFromConstraints can create a new constraint that a subsequent rule removes. The example in https://issues.apache.org/jira/browse/SPARK-21652 required running ConstantPropagation, ConstantFolding and then BooleanSimplification to remove such an inferred constraint/filter.
I think if we combine InferFiltersFromConstraints with other rules that doesn't reduce constraints (like PushDownPredicates) then we are still good. But I might be wrong so please share your thoughts on this.

Sidebar:
I think the source of the above mentioned loop is that ConstantPropagation doesn't propagate constants into a join condition (it handles filters only). I had an old PR to enhance that rule where I also commented why it is not so simple to propagate constants into a join: https://github.com/apache/spark/pull/24553/files#diff-d43484d56a4d9991066b5c00d12ec2465c75131e055fc02ee7fb6dfd45b5006fR76-R79 but it is doable if we fix https://issues.apache.org/jira/browse/SPARK-30598 (#27309).
But I'm not saying that ConstantPropagation is the only reduction rule that can collide with InferFiltersFromConstraints.

I think if we combine InferFiltersFromConstraints with other rules that doesn't reduce constraints (like PushDownPredicates) then we are still good. But I might be wrong so please share your thoughts on this.

Yea, I don't have a specific query to deny your thought and I also think it is true, but, in my current feeling, I a bit hesitate to change Once -> fixedPoint for this batch without adding no logic to avoid the infinite loop case (and without any evidence that it will not affect users queries...). WDTY, @cloud-fan @viirya ?

I initially looked at trying to see if InferFiltersFromConstraints could itself handle this, but I think I could not figure out a way to solve the case in the unit test. I also don't think that this can cause an infinite loop, but let me take another look at this to see if we can either be sure that it cannot result in an infinite loop or if we can push this into InferFiltersFromConstraints so that we don't need the fixedPoint.

Not look at InferFiltersFromConstraints, but I tend to agree with @maropu's initial comment. Can we possibly make InferFiltersFromConstraints deal with this kind of case?

ahshahid · 2020-11-10T18:32:43Z

I have proposed a PR for the bug SPARK-33152, which proposes a new logic for maintaining constraint propagation which I have seen is able to infer filters / prune filters more rigorously. I think that PR should solve this issue too.

github-actions · 2021-02-19T00:44:46Z

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!

probot-autolabeler bot added the SQL label Jul 20, 2020

maropu reviewed Jul 21, 2020

View reviewed changes

maropu changed the title ~~[SPARK-30876][SQL]: Optimizer fails to infer constraints within join~~ [SPARK-30876][SQL] Optimizer fails to infer constraints within join Jul 22, 2020

navinvishy added 2 commits July 23, 2020 18:02

Remove unused code

51977bc

Fix import order

2b3c82a

maropu reviewed Nov 6, 2020

View reviewed changes

github-actions bot added the Stale label Feb 19, 2021

github-actions bot closed this Feb 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-30876][SQL] Optimizer fails to infer constraints within join #29170

[SPARK-30876][SQL] Optimizer fails to infer constraints within join #29170

navinvishy commented Jul 20, 2020

maropu Jul 21, 2020

maropu commented Jul 21, 2020

maropu commented Jul 21, 2020

SparkQA commented Jul 21, 2020

maropu commented Jul 21, 2020

SparkQA commented Jul 21, 2020

maropu commented Jul 21, 2020

SparkQA commented Jul 21, 2020

maropu commented Jul 22, 2020

SparkQA commented Jul 22, 2020

navinvishy commented Jul 22, 2020

SparkQA commented Jul 24, 2020

SparkQA commented Jul 24, 2020

maropu commented Jul 27, 2020

maropu commented Jul 27, 2020

SparkQA commented Jul 28, 2020

navinvishy commented Aug 1, 2020

peter-toth commented Nov 3, 2020

maropu commented Nov 6, 2020

maropu Nov 6, 2020

peter-toth Nov 6, 2020

maropu Nov 9, 2020

navinvishy Nov 9, 2020

viirya Nov 9, 2020

ahshahid commented Nov 10, 2020

github-actions bot commented Feb 19, 2021

[SPARK-30876][SQL] Optimizer fails to infer constraints within join #29170

[SPARK-30876][SQL] Optimizer fails to infer constraints within join #29170

Conversation

navinvishy commented Jul 20, 2020

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?

maropu Jul 21, 2020

Choose a reason for hiding this comment

maropu commented Jul 21, 2020

maropu commented Jul 21, 2020

SparkQA commented Jul 21, 2020

maropu commented Jul 21, 2020

SparkQA commented Jul 21, 2020

maropu commented Jul 21, 2020

SparkQA commented Jul 21, 2020

maropu commented Jul 22, 2020

SparkQA commented Jul 22, 2020

navinvishy commented Jul 22, 2020

SparkQA commented Jul 24, 2020

SparkQA commented Jul 24, 2020

maropu commented Jul 27, 2020

maropu commented Jul 27, 2020

SparkQA commented Jul 28, 2020

navinvishy commented Aug 1, 2020

peter-toth commented Nov 3, 2020

maropu commented Nov 6, 2020

maropu Nov 6, 2020

Choose a reason for hiding this comment

peter-toth Nov 6, 2020

Choose a reason for hiding this comment

maropu Nov 9, 2020

Choose a reason for hiding this comment

navinvishy Nov 9, 2020

Choose a reason for hiding this comment

viirya Nov 9, 2020

Choose a reason for hiding this comment

ahshahid commented Nov 10, 2020

github-actions bot commented Feb 19, 2021