fix(parquet): support mask filtering across skipped pages#10288
Merged
Conversation
hhhizzz
marked this pull request as draft
July 5, 2026 15:43
hhhizzz
changed the title
hhhizzz
marked this pull request as ready for review
July 9, 2026 16:38
hhhizzz
changed the title
Contributor
Author
|
@alamb 👋Hi, This PR is now ready for review and updated with the latest main. |
Contributor
|
Ok, thank ou @hhhizzz -- I will try. It just takes me a qhile to review non trivial PRs like this (not just the code, but also the approach). |
Contributor
Author
Thanks, and no rush at all. Please take whatever time works for you. I completely understand that reviewing the overall approach of a non-trivial change takes significantly more time than just reviewing the code. I’ve been focusing on this area recently, and I added a few diagrams to the PR description in the hope that they make the design and execution flow easier to understand. |
hhhizzz
marked this pull request as draft
July 16, 2026 14:33
hhhizzz
changed the title
hhhizzz
force-pushed
the
fix/parquet-mask-sparse-pages
branch
from
July 16, 2026 14:38
fd2a99e to
4fef178
Compare
Accumulate loaded-range-safe mask chunks until batch_size so sparse page pruning does not fragment logical batches or predicate evaluation. Cover cache policies, initial selections, multiple predicates, and nested columns with different page boundaries.
hhhizzz
changed the title
hhhizzz
marked this pull request as ready for review
July 17, 2026 02:00
Contributor
Author
|
@alamb 👋 A brief update since your last note: I simplified the implementation I also realized that, in earlier iterations, I let the scope grow by pursuing I have now narrowed this PR to one behavioral goal: making Mask execution The PR has 604 additions and 168 deletions overall, but when Whenever your schedule allows, I’d really appreciate your review of the |
Contributor
|
I am really sorry -- I will do this one first thing tomorrow morning |
Contributor
Thank you very much for this offer -- as you can tell we sorely need more reviewers for this code (which is One I think that looks particularly promising and quite related is this one by @haohuaijin ; Perhaps you can find some time to help with that |
Contributor
I agree with this. As you have seen the bandwidth we have to review code in this repo is quite limited (the same I think applies to pretty much every open source project). Thus, while it is unfortunate, expending extra effort as a contributor to make the changes easy to review is often the best way to get a contribution reviewed sooner
Thank you
I agree this will hopefully improve the velocity at which we can review and merge your PRs |
Contributor
|
I have begun my review, but have had to spend more time on a release this morning than I had planned |
Contributor
Author
Thank you! At the same time I'm also working on reviewing #10141 |
Contributor
|
run benchmark arrow_reader arrow_reader_row_filter |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/parquet-mask-sparse-pages (152d581) to 980667f (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/parquet-mask-sparse-pages (152d581) to 980667f (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
I also run the TPCDS on my machine, the result is around same, this change only fix some rare situation that using mask on pruned page which shouldn't impact the performance. |
alamb
reviewed
Jul 19, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you @hhhizzz -- I spent quite a while studying this PR and I found it very well structured and a very clever design
In order to merge it, I think we need a few more tests to cover some edge cases (I left specific comments)
I also think it would help a lot to document some of the very clean, though complex design of RowSelectionCursor and MaskCursor (I left some specific suggestions inline as well), though I think we could do this as a follow onn
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Make the trailing-skip invariant explicit and remove unreachable empty-chunk handling. Extract mask accumulation from next_inner and document how loaded ranges coalesce into one logical batch.
Contributor
Author
Thanks for the careful review. I have addressed the coverage and readability points locally: the two unreachable branches are gone, the trailing-skip invariant now has dedicated tests, the Mask path has been extracted from I also reran the suggested llvm-cov command. The full parquet test suite passed, and the newly extracted cursor and accumulator paths are covered. |
alamb
approved these changes
Jul 20, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you @hhhizzz -- think this looks great. I went through the code and tests again and it is looking 👌
I took the liberty of pushing a commit that has a more fully worked example in comments for FilterMaskAccumulator and merging up from main.
| read_plan: ReadPlan, | ||
| } | ||
|
|
||
| /// Accumulates filter masks for decoded chunks in one logical output batch. |
Contributor
There was a problem hiding this comment.
Thank you for this-- I found it much clearer
| return Ok(None); | ||
| } | ||
|
|
||
| let filter_mask = filter_mask |
Contributor
There was a problem hiding this comment.
thank you -- this now ready very clearly to me
Contributor
|
I also am running some performance tests: |
Contributor
|
Performance tests looks good so I'll merge this one in Thank you again @hhhizzz |
haohuaijin
added a commit
to haohuaijin/arrow-rs
that referenced
this pull request
Jul 21, 2026
Resolves conflicts with apache#10288 (mask filtering across skipped pages): - Port LoadedRowRanges-aware MaskCursor (next_chunk / next_mask_chunk_non_empty) into selection/boolean.rs - Thread loaded_row_ranges into both RowSelectionCursor::new_mask_from_buffer and new_mask_from_selectors - Keep LoadedRowRanges in selection/mod.rs
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
3 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.
Which issue does this PR close?
Rationale for this change
Mask-backed row selection decodes every row covered by a mask chunk before applying Arrow's filter kernel. Page pruning can leave sparse in-memory column chunks where complete pages were never fetched. If a mask chunk crosses one of those gaps, decoding requests data that is not present and fails with an invalid sparse-page offset.
Previously,
Autoavoided this failure by falling back to selectors when page pruning was detected, while an explicitRowSelectionPolicy::Maskcould still fail. This PR makes skipped pages safe for Mask execution, allowing explicit Mask to work andAutoto retain its density-based strategy choice.What changes are included in this PR?
The reader now computes
LoadedRowRanges: absolute row-group intervals whose backing pages are loaded for every Parquet leaf in the current projection.For each predicate projection and the final output projection, it:
skip_records()to cross gaps without decoding missing pages.These ranges are decoder safety boundaries, not output batch boundaries.
ParquetRecordBatchReaderaccumulates multiple safe chunks and their mask bits until it reachesbatch_size, then consumes and filters once. Sparse pages therefore do not fragment one logical batch into one batch per loaded range.When no projected pages were skipped,
LoadedRowRangesis omitted and the existing Mask fast path is unchanged. No masks are intersected or rewritten; the original selection mask is still applied by Arrow's filter kernel.Loaded row range example
Consider two projected columns with different page boundaries and the selection mask
100000000001:[0, 4)[4, 6)[6, 8)[8, 10)[10, 12)100000000001[4, 8)[8, 12)[0, 6)[6, 10)The intersection does not need to match any one column's page boundaries. It is a row-domain guarantee: every row in the result has backing page data in every projected leaf. For example,
[0, 4)is a valid subset of Column B's[0, 6)page.Mask execution and batch behavior
With
batch_size = 12, the reader processes the example as follows:[0, 4)with mask1000skip_records(7)to row 11, then read[11, 12)with mask1The unloaded gap is never decoded, while the loaded-range boundary remains invisible to callers as a batch boundary.
Are these changes tested?
Coverage includes:
Auto, with predicate caching enabled and disabled;List<Struct<...>>projections whose leaves have different page boundaries;Validated with:
The current GitHub CI run is green, including Parquet tests, Clippy, MSRV, macOS, Windows, wasm32, and PySpark integration.
Are there any user-facing changes?
There are no public API or source compatibility changes.
Explicit Mask execution no longer fails when page pruning creates sparse column data.
Autono longer falls back to selectors solely because pages were pruned, and sparse loaded ranges no longer fragment logical output batches. These changes can affect execution performance characteristics, but not output semantics.