Conditionally build page pruning predicates#21480
Open
fpetkovski wants to merge 1 commit intoapache:mainfrom
Open
Conditionally build page pruning predicates#21480fpetkovski wants to merge 1 commit intoapache:mainfrom
fpetkovski wants to merge 1 commit intoapache:mainfrom
Conversation
Page pruning predicates in the Parquet opener are constructed regardless of whether enable_page_index is set. Under high query load, this uses significant CPU time although these predicates are created and discarded quickly. This commit reorders the predicate creation flow to only construct page pruning predicates if enable_page_index is enabled. Regular predicates are created always as before.
fpetkovski
force-pushed
the
optimize-pruning-predicate
branch
from
cfcc878 to
e5905e2
Compare
alamb
reviewed
Apr 8, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @fpetkovski -- this is a good idea
Do you think there is any way to test this? otherwise we run a real danger of regressing performance under refactors (for example the one I am working on now with #21327
Though it is not clear to me what such a test would look like / how to do it without causing non trivial churn 🤔
Contributor
|
This removes a redundant step, and the change looks reasonable. However, I’m curious how this step ends up consuming significant CPU time — building a predicate per file doesn’t seem very expensive. Do you have a micro-benchmark that demonstrates the impact on a representative workload? |
Contributor
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing optimize-pruning-predicate (e5905e2) to 91c2e04 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing optimize-pruning-predicate (e5905e2) to 91c2e04 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing optimize-pruning-predicate (e5905e2) to 91c2e04 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
Dandandan
reviewed
Apr 9, 2026
|
|
||
| // Build predicates for this specific file | ||
| let (pruning_predicate, page_pruning_predicate) = build_pruning_predicates( | ||
| let pruning_predicate = build_pruning_predicates( |
Contributor
There was a problem hiding this comment.
Maybe better is to delay it so they are only created after we loaded the page index, this avoids doing it as well for files without page index (and avoids it in short-circuit scenarios / query cancellation).
Contributor
There was a problem hiding this comment.
I can help do this as a follow on PR too
Author
|
Thanks everyone for the review. I will address comments and in the meantime I have attached a profile screenshot from our production in the PR description. We query many small files very frequently (thousands of QPS), which are already pre-filtered outside of datafusion so pruning predicates have little effect. As can be seen, half of query time is spent opening files. |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
|
Maybe the predicate is complex 🤔 |
alamb
approved these changes
Apr 9, 2026
|
|
||
| // Build predicates for this specific file | ||
| let (pruning_predicate, page_pruning_predicate) = build_pruning_predicates( | ||
| let pruning_predicate = build_pruning_predicates( |
Contributor
There was a problem hiding this comment.
I can help do this as a follow on PR too
Contributor
|
We need to clean up CI and ideally find some sort of tests for this, but I think it is an improvement over main -- thank you @fpetkovski |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Rationale for this change
Page pruning predicates in the Parquet opener are constructed regardless of whether enable_page_index is set. Under high query load, this uses significant CPU time although these predicates are created and discarded quickly.
Which issue does this PR close?
What changes are included in this PR?
This commit reorders the predicate creation flow to only construct page pruning predicates if enable_page_index is enabled. Regular predicates are created always as before.
Are these changes tested?
I am relying on unit tests but I can do manual testing with a debugger.
Are there any user-facing changes?
Changes are are optimization and are not user-facing.