-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-25699][SQL] Partially push down conjunctive predicated in ORC #22684
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
Conversation
| )).get.toString | ||
| } | ||
|
|
||
| // Safely remove unsupported `StringContains` predicate and push down `LessThan` |
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.
There is another predicate pushed down. We shall mention it too?
|
Test build #97187 has finished for PR 22684 at commit
|
|
Test build #97199 has finished for PR 22684 at commit
|
| val leftBuilderOption = createBuilder(dataTypeMap, left, | ||
| newBuilder, canPartialPushDownConjuncts) | ||
| val rightBuilderOption = | ||
| createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts) |
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, can you make the format the same as leftBuilderOption? Also, add another empty line before (leftBuilderOption, rightBuilderOption). Thanks.
| _ <- buildSearchArgument(dataTypeMap, child, newBuilder) | ||
| negate <- buildSearchArgument(dataTypeMap, child, builder.startNot()) | ||
| _ <- createBuilder(dataTypeMap, child, newBuilder, canPartialPushDownConjuncts = false) | ||
| negate <- createBuilder(dataTypeMap, child, builder.startNot(), false) |
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, can you explicitly call canPartialPushDownConjuncts = false?
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.
Then it has to be two lines..I am OK with that too.
| newBuilder, canPartialPushDownConjuncts) | ||
| val rightBuilderOption = | ||
| createBuilder(dataTypeMap, right, newBuilder, canPartialPushDownConjuncts) | ||
| (leftBuilderOption, rightBuilderOption) match { |
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.
ditto
| _ <- buildSearchArgument(dataTypeMap, child, newBuilder) | ||
| negate <- buildSearchArgument(dataTypeMap, child, builder.startNot()) | ||
| _ <- createBuilder(dataTypeMap, child, newBuilder, canPartialPushDownConjuncts = false) | ||
| negate <- createBuilder(dataTypeMap, child, builder.startNot(), false) |
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.
ditto
|
LGTM. Just some styling feedback. Thanks. |
|
Merged into master. Thanks. |
|
Test build #97209 has finished for PR 22684 at commit
|
## What changes were proposed in this pull request? Inspired by apache#22574 . We can partially push down top level conjunctive predicates to Orc. This PR improves Orc predicate push down in both SQL and Hive module. ## How was this patch tested? New unit test. Closes apache#22684 from gengliangwang/pushOrcFilters. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: DB Tsai <d_tsai@apple.com>
What changes were proposed in this pull request?
Inspired by #22574 .
We can partially push down top level conjunctive predicates to Orc.
This PR improves Orc predicate push down in both SQL and Hive module.
How was this patch tested?
New unit test.