You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "haohuaijin (via GitHub)" <gi...@apache.org> on 2026/07/25 03:56:49 UTC
[PR] refactor(parquet): split `arrow_reader/selection` into smaller modules [arrow-rs]
haohuaijin opened a new pull request, #10434:
URL: https://github.com/apache/arrow-rs/pull/10434
# Which issue does this PR close?
- Closes #10424.
# Rationale for this change
Follow up to #10141. `parquet/src/arrow/arrow_reader/selection/mod.rs` had grown past 2100 lines and mixed several concerns: the `RowSelection` type and its public API, the set algebra helpers, the page range mapping, the cursor machinery and a large test module.
This is a mechanical reorganization to make the code easier to navigate and review.
# What changes are included in this PR?
The module is split by concern, with one module per `RowSelection` backing so the two are symmetric:
| module | contents |
| --- | --- |
| `mod.rs` | `RowSelection`, `RowSelectionInner` and the public API, dispatching on the backing |
| `selector.rs` | run length backing: `RowSelector`, `RowSelectionIter` and the selector primitives |
| `boolean.rs` | bitmap backing: `MaskSelection`, `MaskRunIter` and the mask primitives |
| `algebra.rs` | `and_then`, `intersection`, `union` for both backings |
| `ranges.rs` | page byte ranges and batch boundary expansion |
| `cursor.rs` | `RowSelectionCursor`, `MaskCursor`, `SelectorsCursor`, `LoadedRowRanges`, `RowSelectionPolicy` / `RowSelectionStrategy` |
The two backings now mirror each other:
| operation | `selector.rs` | `boolean.rs` |
| --- | --- | --- |
| storage | `Vec<RowSelector>` | `MaskSelection` |
| iteration | `RowSelectionIter` | `MaskRunIter` |
| construction | `combine_selectors`, `selectors_from_consecutive_ranges` | `boolean_mask_from_selectors` |
| split / trim / offset / limit | `*_selectors` | `*_mask` |
so each `RowSelection` method is a short dispatch on `RowSelectionInner`, for example:
```rust
pub(crate) fn limit(self, limit: usize) -> Self {
match self.inner {
RowSelectionInner::Mask(mask) => { /* limit_mask + count bookkeeping */ }
RowSelectionInner::Selectors(selectors) => {
Self::from_selectors(limit_selectors(selectors, limit))
}
}
}
```
Function bodies are moved verbatim. The only shape changes are:
* the selector helpers return `Vec<RowSelector>` instead of `RowSelection`, so the wrapping happens once at the call site;
* `RowSelectionIter` gained a `pub(super) fn new`, as its tuple field is no longer visible from `mod.rs`;
* `and_then_selectors_with_mask` was added to `algebra.rs` (a 6 line wrapper) so that `and_then_iter` can stay private to that module.
# Are these changes tested?
Yes, by the existing tests: they were moved next to the code they now cover (selector mechanics to `selector.rs`, algebra to `algebra.rs`, `scan_ranges` to `ranges.rs`, cursor tests to `cursor.rs`), but no test was added, removed or renamed — the set of test names under `arrow::arrow_reader::selection` is identical before and after this PR.
# 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 outside `selection/` needed modification.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(parquet): split `arrow_reader/selection` into smaller modules [arrow-rs]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on code in PR #10434:
URL: https://github.com/apache/arrow-rs/pull/10434#discussion_r3650173487
##########
parquet/src/arrow/arrow_reader/selection/mod.rs:
##########
@@ -491,21 +304,6 @@ impl RowSelection {
}
}
- /// 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!(),
- }
- }
-
Review 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;
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(parquet): split `arrow_reader/selection` into smaller modules [arrow-rs]
Posted by "haohuaijin (via GitHub)" <gi...@apache.org>.
haohuaijin commented on PR #10434:
URL: https://github.com/apache/arrow-rs/pull/10434#issuecomment-5078495832
Pure reorganization of `arrow_reader/selection: mod.rs` had grown past 2200 lines and is now split by concern. Bodies are moved verbatim, the public API is unchanged — the diff should read as a move.
cc @alamb @hhhizzz — follow up to #10141, would appreciate a look when you have time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(parquet): split `arrow_reader/selection` into smaller modules [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10434:
URL: https://github.com/apache/arrow-rs/pull/10434#discussion_r3651177385
##########
parquet/src/arrow/arrow_reader/selection/mod.rs:
##########
@@ -15,83 +15,48 @@
// specific language governing permissions and limitations
// under the License.
+//! Logic for selecting which rows to read: [`RowSelection`] and [`RowSelector`]
+//!
+//! 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
Review Comment:
❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(parquet): split `arrow_reader/selection` into smaller modules [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10434:
URL: https://github.com/apache/arrow-rs/pull/10434
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org