-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-13668][SQL] Reorder filter/join predicates to short-circuit isNotNull checks #11511
Conversation
Test build #52447 has finished for PR 11511 at commit
|
|
||
// Attempts to re-order the individual conjunctive predicates in an expression to short circuit | ||
// the evaluation of relatively cheaper checks (e.g., checking for nullability) before others. | ||
protected def getReorderedExpression(expr: Expression): Expression = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i think this is more aptly named "reorderPredicates"
LGTM |
d75d313
to
4a8a9a3
Compare
Thanks, comments addressed. |
Test build #52487 has finished for PR 11511 at commit
|
Test build #52488 has finished for PR 11511 at commit
|
|
||
// Attempts to re-order the individual conjunctive predicates in an expression to short circuit | ||
// the evaluation of relatively cheaper checks (e.g., checking for nullability) before others. | ||
protected def reorderPredicates(expr: Expression): Expression = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a test to make sure this reordering is stable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I modified the 2 tests in ReorderPredicateSuite
to verify that the sort is stable.
4a8a9a3
to
21d8897
Compare
Test build #52692 has finished for PR 11511 at commit
|
LGTM. Merging to master. |
…NotNull checks ## What changes were proposed in this pull request? If a filter predicate or a join condition consists of `IsNotNull` checks, we should reorder these checks such that these non-nullability checks are evaluated before the rest of the predicates. For e.g., if a filter predicate is of the form `a > 5 && isNotNull(b)`, we should rewrite this as `isNotNull(b) && a > 5` during physical plan generation. ## How was this patch tested? new unit tests that verify the physical plan for both filters and joins in `ReorderedPredicateSuite` Author: Sameer Agarwal <sameer@databricks.com> Closes apache#11511 from sameeragarwal/reorder-isnotnull.
What changes were proposed in this pull request?
If a filter predicate or a join condition consists of
IsNotNull
checks, we should reorder these checks such that these non-nullability checks are evaluated before the rest of the predicates.For e.g., if a filter predicate is of the form
a > 5 && isNotNull(b)
, we should rewrite this asisNotNull(b) && a > 5
during physical plan generation.How was this patch tested?
new unit tests that verify the physical plan for both filters and joins in
ReorderedPredicateSuite