[SPARK-17244] Catalyst should not pushdown non-deterministic join conditions by sameeragarwal · Pull Request #14815 · apache/spark

sameeragarwal · 2016-08-25T20:54:32Z

What changes were proposed in this pull request?

Given that non-deterministic expressions can be stateful, pushing them down the query plan during the optimization phase can cause incorrect behavior. This patch fixes that issue by explicitly disabling that.

How was this patch tested?

A new test in FilterPushdownSuite that checks catalyst behavior for both deterministic and non-deterministic join conditions.

sameeragarwal · 2016-08-25T20:54:43Z

cc @hvanhovell @gatorsmile

sameeragarwal · 2016-08-25T20:55:41Z

cc @brkyvz who found this bug

gatorsmile · 2016-08-25T22:14:34Z

@@ -1386,15 +1386,17 @@ object EliminateOuterJoin extends Rule[LogicalPlan] with PredicateHelper {
 object PushPredicateThroughJoin extends Rule[LogicalPlan] with PredicateHelper {
 /**
 * Splits join condition expressions into three categories based on the attributes required


It sounds like this PR is targeting both join conditions and filter predicates. Could you update the code comment of this line and the title of the PR?

SparkQA · 2016-08-25T22:36:36Z

Test build #64436 has finished for PR 14815 at commit 6728fc3.

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

SparkQA · 2016-08-25T23:25:56Z

Test build #64437 has finished for PR 14815 at commit d0b1009.

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

sameeragarwal · 2016-08-26T18:14:49Z

Thanks @gatorsmile, I address your comments.

gatorsmile · 2016-08-26T18:34:11Z

Looks pretty good to me!

Could you also add one more test case for testing the following scenario:

only consider pushing down those expressions that precede the first non-deterministic expression in the condition.

I am just afraid the others might break it in the future. Thanks!

sameeragarwal · 2016-08-26T18:42:38Z

That's a good idea. More generally, I think the best way to fix these class of problems is to unify all predicate pushdown logic at a common place.

SparkQA · 2016-08-26T20:21:08Z

Test build #64496 has finished for PR 14815 at commit f86f5ce.

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

gatorsmile · 2016-08-26T20:29:49Z

In 2.0, we combined the PPD rules. Now, only two PPD rules left, PushPredicateThroughJoin and PushDownPredicate. IMO, the major reason why we did not combine them into the same one is because PPD for Join is a little bit special.

gatorsmile · 2016-08-26T22:06:12Z

LGTM pending testing

SparkQA · 2016-08-26T22:57:36Z

Test build #64500 has finished for PR 14815 at commit b326cd8.

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

yhuai · 2016-08-26T23:40:19Z

LGTM. Merging to master and branch 2.0.

…ditions ## What changes were proposed in this pull request? Given that non-deterministic expressions can be stateful, pushing them down the query plan during the optimization phase can cause incorrect behavior. This patch fixes that issue by explicitly disabling that. ## How was this patch tested? A new test in `FilterPushdownSuite` that checks catalyst behavior for both deterministic and non-deterministic join conditions. Author: Sameer Agarwal <sameerag@cs.berkeley.edu> Closes#14815 from sameeragarwal/constraint-inputfile. (cherry picked from commit 540e912) Signed-off-by: Yin Huai <yhuai@databricks.com>

cloud-fan · 2016-08-27T07:15:48Z

- * to evaluate them.
+ * Splits join condition expressions or filter predicates (on a given join's output) into three
+ * categories based on the attributes required to evaluate them. Note that we explicitly exclude
+ * on-deterministic (i.e., stateful) condition expressions in canEvaluateInLeft or


typo: non-deterministic

good eye! can you please fold this change into one of your open PRs :) ?

yea, I will :)

sameeragarwal added 2 commits August 25, 2016 13:45

Joins should not pushdown non-deterministic conditions
9515097

unit test
6728fc3

oops
d0b1009

gatorsmile reviewed Aug 25, 2016
View reviewed changes

CR
f86f5ce

unit test
b326cd8

asfgit closed this in 540e912Aug 26, 2016

cloud-fan reviewed Aug 27, 2016
View reviewed changes

cloud-fan mentioned this pull request Aug 6, 2020
[SPARK-32528][SQL][TEST] The analyze method should make sure the plan is analyzed #29349
Closed

Uh oh!

Conversation

sameeragarwal commented Aug 25, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

sameeragarwal commented Aug 25, 2016

Uh oh!

sameeragarwal commented Aug 25, 2016

Uh oh!

gatorsmileAug 25, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 25, 2016

Uh oh!

SparkQA commented Aug 25, 2016

Uh oh!

sameeragarwal commented Aug 26, 2016

Uh oh!

gatorsmile commented Aug 26, 2016

Uh oh!

sameeragarwal commented Aug 26, 2016

Uh oh!

SparkQA commented Aug 26, 2016

Uh oh!

gatorsmile commented Aug 26, 2016

Uh oh!

gatorsmile commented Aug 26, 2016

Uh oh!

SparkQA commented Aug 26, 2016

Uh oh!

yhuai commented Aug 26, 2016

Uh oh!

cloud-fanAug 27, 2016• 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!

sameeragarwalAug 27, 2016

Choose a reason for hiding this comment

Uh oh!

cloud-fanAug 27, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cloud-fanAug 27, 2016•
edited
Loading