[SPARK-20246][SQL] should not push predicate down through aggregate with non-deterministic expressions #17562
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cloud-fan
commented
Apr 7, 2017
| @@ -134,15 +134,20 @@ class FilterPushdownSuite extends PlanTest { | |||
| comparePlans(optimized, correctAnswer) | |||
| } | |||
|
|
|||
| test("nondeterministic: can't push down filter with nondeterministic condition through project") { | |||
There was a problem hiding this comment.
This test was wrong, actually we can push down nondeterministic filter through project, as long as the project list is all deterministic.
viirya
reviewed
Apr 7, 2017
| @@ -134,15 +134,20 @@ class FilterPushdownSuite extends PlanTest { | |||
| comparePlans(optimized, correctAnswer) | |||
| } | |||
|
|
|||
| test("nondeterministic: can't push down filter with nondeterministic condition through project") { | |||
| test("nondeterministic: can push down nondeterministic filter through project") { | |||
There was a problem hiding this comment.
nit: through project with all deterministic fields.
|
LGTM except a minor comment. |
|
Test build #75597 has finished for PR 17562 at commit
|
|
Test build #75598 has finished for PR 17562 at commit
|
|
Test build #75600 has finished for PR 17562 at commit
|
gatorsmile
reviewed
Apr 7, 2017
| @@ -792,7 +793,8 @@ object PushDownPredicate extends Rule[LogicalPlan] with PredicateHelper { | |||
| filter | |||
| } | |||
|
|
|||
| case filter @ Filter(condition, aggregate: Aggregate) => | |||
| case filter @ Filter(condition, aggregate: Aggregate) | |||
| if aggregate.aggregateExpressions.forall(_.deterministic) => | |||
There was a problem hiding this comment.
Could you move this case above case filter @ Filter(condition, w: Window)?
Based on the comment you add above, it becomes easier to follow by the readers.
|
LGTM except a minor comment |
|
Test build #75604 has finished for PR 17562 at commit
|
asfgit
pushed a commit
that referenced
this pull request
Apr 8, 2017
…ith non-deterministic expressions ## What changes were proposed in this pull request? Similar to `Project`, when `Aggregate` has non-deterministic expressions, we should not push predicate down through it, as it will change the number of input rows and thus change the evaluation result of non-deterministic expressions in `Aggregate`. ## How was this patch tested? new regression test Author: Wenchen Fan <wenchen@databricks.com> Closes #17562 from cloud-fan/filter. (cherry picked from commit 7577e9c) Signed-off-by: Xiao Li <gatorsmile@gmail.com>
asfgit
pushed a commit
that referenced
this pull request
Apr 8, 2017
…ith non-deterministic expressions ## What changes were proposed in this pull request? Similar to `Project`, when `Aggregate` has non-deterministic expressions, we should not push predicate down through it, as it will change the number of input rows and thus change the evaluation result of non-deterministic expressions in `Aggregate`. ## How was this patch tested? new regression test Author: Wenchen Fan <wenchen@databricks.com> Closes #17562 from cloud-fan/filter. (cherry picked from commit 7577e9c) Signed-off-by: Xiao Li <gatorsmile@gmail.com>
|
Thanks! Merging to master/2.1/2.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Similar to
Project, whenAggregatehas non-deterministic expressions, we should not push predicate down through it, as it will change the number of input rows and thus change the evaluation result of non-deterministic expressions inAggregate.How was this patch tested?
new regression test