Repository navigation
perf: don't build a parquet stream for a file range with no row group left - #26476
Merged
alamb merged 3 commits intoOct 22, 2026
Merged
Conversation
|
|
||
| /// Drives a [`ParquetMorselizer`] to completion and returns how many | ||
| /// morsels it produced. | ||
| async fn count_morsels( |
Contributor
Author
There was a problem hiding this comment.
Only used by tests, I could put a #[cfg(test)] on it?
This also is a bit hacky but I opted not to care because it was just for tests but happy to clean it up a bit :)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26476 +/- ##
=======================================
Coverage 82.90% 82.90%
=======================================
Files 1154 1154
Lines 457654 457729 +75
Branches 457654 457729 +75
=======================================
+ Hits 379398 379465 +67
- Misses 55252 55256 +4
- Partials 23004 23008 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alamb
approved these changes
Oct 20, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me -- thank you @Braedon-Wooding-Displayr
I had a few suggestions
| let mut prepared_row_groups = (*prepared).prune_row_groups()?; | ||
| if prepared_row_groups | ||
| .row_groups | ||
| .row_group_indexes() |
Contributor
There was a problem hiding this comment.
maybe could check with empty()?
|
|
||
| /// Drives a [`ParquetMorselizer`] to completion and returns how many | ||
| /// morsels it produced. | ||
| async fn count_morsels( |
Addresses review feedback on apache#26476: replace the inline row_group_indexes().next().is_none() checks with a named helper. RowGroupAccessPlanFilter::is_empty() cannot be used here because it reports whether the file has zero row groups, not whether every row group has been pruned. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
|
I merged up to get the PR flowing |
alamb
enabled auto-merge
October 22, 2026 19:43
Contributor
Author
|
Sorry for not promptly following up! Thank you @alamb |
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
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.
I've got a few fixes to upstream so just starting with small ones.
Which issue does this PR close?
Rationale for this change
If row-group statistics or Bloom filters prune every row group in a file range, the opener still builds the decoder read plans, the decoder and the row filter, then hands back a stream morsel that yields nothing.
PruneFilealready ends early when file statistics prune the range; this does the same once row-group pruning has.What changes are included in this PR?
PruneWithStatisticsandPruneWithBloomFiltersreturnParquetOpenState::Donewhen no row group survives.What is the testing strategy for this PR?
Testing to ensure that we do actually skip them!
Are there any user-facing changes?
Nope!