-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Skip useless pruning predicates in ParquetExec
#4280
Skip useless pruning predicates in ParquetExec
#4280
Conversation
2b10d68
to
49fb6dd
Compare
ParquetExec
49fb6dd
to
12a2795
Compare
Sorry for the rapid fire PRs @Ted-Jiang and @thinkharderdev -- I figured i would try and keep them small to make review easier |
Co-authored-by: Daniël Heres <danielheres@gmail.com>
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.
Nice catch!
For what it is worth, I found these while updating IOx and some our explain plans show So testing for the win! |
Benchmark runs are scheduled for baseline = 949d5af and contender = a584ff5. a584ff5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
So last night I realized that this PR likely broke predicate pushdown for certain predicates -- specifically since the pushdown filters are taken from the pruning predicate, by dropping the pruning predicate we will ensure no pushdown happens as the filter is no longer available. Rather than back this PR out, however, I think we should forge ahead with properly saving the original predicate, as I have done in #4279 |
Which issue does this PR close?
Draft as it builds on #4278re #4020
Rationale for this change
If a predicate can't be turned into a pruning predicate, it is converted to "true" and will not prune anything. However, the parquet exec will still attempt to read and create statistics in this case which is wasteful (as nothing can be pruned)
After #4278 it is also confusing to see
pruning_predicate=true
in the explain planWhat changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?