refactor(parquet): split arrow_reader/selection into smaller modules - #10434
Merged
Conversation
`selection/mod.rs` had grown past 2100 lines and mixed the `RowSelection` type, the set algebra, the page range mapping and the cursor machinery. Split it by concern, with one module per selection backing: * `mod.rs` - `RowSelection` and its public API, dispatching on backing * `selector.rs` - run length backing: `RowSelector` and its primitives * `boolean.rs` - bitmap backing: `MaskSelection` and its primitives * `algebra.rs` - `and_then`, `intersection` and `union` * `ranges.rs` - page byte ranges and batch boundary expansion * `cursor.rs` - `RowSelectionCursor` and the policy / strategy types No functional change: bodies are moved verbatim and the public API is unchanged.
haohuaijin
marked this pull request as draft
July 25, 2026 07:05
…ction Follow up cleanup on the module split, in `selection/mod.rs`: * `trim` and `split_off` dispatched on the backing with `if let` / `matches!` plus an early return, and had to re-destructure the taken `RowSelectionInner` behind `unreachable!()`. Both now `match` on `self.inner` directly, which drops all three `unreachable!()` arms. * `RowSelection::selectors_mut` is removed. Its only two callers were `trim` and `split_off`, and both returned early on the mask backing, so the `Mask` -> `Selectors` promotion inside it was unreachable. * `split_off_selectors` now takes the `Vec<RowSelector>` by value and returns `(head, tail)`, mirroring `split_off_mask`. The `mem::take` and the trailing `mem::swap` it needed to write the tail back through `&mut` are gone. * `trim_selectors` is inlined back into `trim`: a four line loop only extracted to mirror `trim_mask`. No behavior change; the set of test names is still identical.
haohuaijin
marked this pull request as ready for review
July 25, 2026 11:59
Contributor
Author
haohuaijin
commented
Jul 25, 2026
Comment on lines
-494
to
-508
| /// Promote a mask-backed selection to selector backing in place. | ||
| fn selectors_mut(&mut self) -> &mut Vec<RowSelector> { | ||
| if let RowSelectionInner::Mask(_) = &self.inner { | ||
| let mask = match std::mem::take(&mut self.inner) { | ||
| RowSelectionInner::Mask(m) => m, | ||
| RowSelectionInner::Selectors(_) => unreachable!(), | ||
| }; | ||
| self.inner = RowSelectionInner::Selectors(mask_to_selectors(mask.mask())); | ||
| } | ||
| match &mut self.inner { | ||
| RowSelectionInner::Selectors(s) => s, | ||
| RowSelectionInner::Mask(_) => unreachable!(), | ||
| } | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
remove those code as it only use in trim and split_off, but current trim and split_off now match on self.inner instead of dispatching with if let / matches! plus an early return;
alamb
approved these changes
Jul 25, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you -- This is great @haohuaijin
| //! This module holds [`RowSelection`] and its public API, which dispatches to | ||
| //! one of the two backings depending on how the selection is stored: | ||
| //! | ||
| //! * `selector`: the run length backing, [`RowSelector`] and its primitives |
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
2 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?
parquet/src/arrow/arrow_reader/selection/mod.rsinto smaller modules #10424.Rationale for this change
Follow up to #10141.
selection/mod.rshad grown past 2200 lines and mixed theRowSelectiontype and its public API, the set algebra, the page range mapping, the cursor machinery and a large test module. This is a reorganization to make the code easier to navigate; there is no functional change.What changes are included in this PR?
The module is split by concern, and
mod.rsgoes from 2219 to 947 lines:mod.rsRowSelection,RowSelectionInnerand the public APIselector.rsRowSelector,RowSelectionIter, and thesplit_off/offset/limitprimitivesboolean.rsMaskSelection,MaskRunIterand the mask primitivesalgebra.rsand_then,intersectionandunionfor both backingsranges.rscursor.rsRowSelectionCursor,MaskCursor,SelectorsCursor,LoadedRowRanges,RowSelectionPolicy/RowSelectionStrategyAre these changes tested?
Yes, by the existing tests, moved next to the code they now cover. No test was added, removed or renamed: the set of test names under
arrow::arrow_reader::selectionis identical before and after this PR, and thesplit_off/trim/offset/limittests still go through theRowSelectionmethods rather than the extracted helpers.Are there any user-facing changes?
No. The public API (
parquet::arrow::arrow_reader::{RowSelection, RowSelector, RowSelectionCursor, RowSelectionPolicy, RowSelectionIter, MaskRunIter}) is unchanged, and no call site outsideselection/needed modification.One internal note:
SelectorsCursorandMaskChunkare no longer nameable outsideselection/, as they moved into the privatecursormodule and are not re-exported. Neither was reachable from outside the crate before, sinceselectionispub(crate).