feat(parquet): RowSelection can be backed by a BooleanBuffer - #10141
Conversation
|
Thanks @alamb for the pointers. I looked at #6624, #7454, and the work that landed in #8733. My understanding is:
This does not replace selector-backed selections. Selectors are still better for clustered/page-index-style selections. The case this helps is when the caller already has a row-level bitmap, such as from an external index, FTS index, or bitmap index. Today that bitmap has to go through So I see this as a small representation-layer improvement that complements #8733. #8733 added mask execution; this PR adds a direct bitmap-backed input path for it. It does not try to implement the broader adaptive predicate-pushdown/page-cache design from #7454. |
|
run benchmark arrow_reader arrow_reader_row_filter |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing export-mask (1720537) to 2e035fd (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing export-mask (1720537) to 2e035fd (merge-base) diff File an issue against this benchmark runner |
|
run benchmark arrow_reader_clickbench |
|
Merging up to fix CI |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing export-mask (7b102f4) to 6ba533d (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 |
|
run benchmark row_selector row_selection_cursor |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @haohuaijin . I think this is totally the right direction and I really like where this is going
As long as this doesn't cause a performance regression I think it is good to go.
I have actually tried this myself (see #6624) as did @XiangpengHao in #6624.
Some other thoughts
- We need to make sure it doesn't cause a performance regression (I can help here)
- It currently has a public API change, so we can't merge it until main opens for the next major release (likely in mid-late July). If we can figure out how to keep the API the same we could potentially release it sooner
I haven' tmade it through the entire PR yet, but I did fire off some benchmarks
|
|
||
| #[inline] | ||
| fn next(&mut self) -> Option<RowSelector> { | ||
| match self { |
There was a problem hiding this comment.
In general I think this will add a branch in the main loops - every chunk of bits will require matching -- it might make sense to figure out how to have iterators for the two different types, so that code paths that want to iterate over the selection can match outside
match iter {
RowSelectionIter::Selectors(iter) => { for i in iter ... }, // special loop for selector backed
RowSelectionIter::Mask(iter) => { for i in iter ... }, // special loop for mask backed
}| if self.finished { | ||
| return None; | ||
| } | ||
| match self.slices.next() { |
There was a problem hiding this comment.
Much of this code appears untested
I ran code coverage like
cargo llvm-cov --html test -p parquet --features arrow
| impl Iterator for MaskRunIter<'_> { | ||
| type Item = RowSelector; | ||
|
|
||
| fn next(&mut self) -> Option<RowSelector> { |
There was a problem hiding this comment.
I think this is the main API that is not compatible -- it need to return &RowSelector -- if we could change that we can merge this PR before the next major release (with breaking API changes)
There was a problem hiding this comment.
I updated RowSelection::iter() to keep returning Iterator<Item = &RowSelector>.
- For selector-backed selections this still borrows from the internal
Vec<RowSelector>. - For mask-backed selections I added a lazy selector cache, because the returned
&RowSelectorvalues need stable storage insideRowSelection; otherwise they would point into a temporaryVec.
The internal hot paths still avoid this cache and use the BooleanBuffer or MaskRunIter directly.
the struct is below
pub struct RowSelection {
inner: RowSelectionInner,
}
pub(crate) enum RowSelectionInner {
Selectors(Vec<RowSelector>),
Mask(Box<MaskSelection>),
}
pub(crate) struct MaskSelection {
mask: BooleanBuffer,
selectors: OnceLock<Vec<RowSelector>>,
}I am not fully sure this is the best approach, so suggestions and feedback are welcome.
| if let (Some(l), Some(r)) = (self.as_mask(), other.as_mask()) { | ||
| return Self::from_boolean_buffer(intersect_masks(l, r)); | ||
| } | ||
| let l = self.materialize_for_combine(); |
There was a problem hiding this comment.
this is a clever idea -- as a follow on we could potentially implement specialized versions of each of these implementations
There was a problem hiding this comment.
for example, I think and_then can use some of the fancy BMI instructions that @devanbenz is investigating in #10136
| // Verify that the size of RowGroupDecoderState does not grow too large | ||
| fn test_structure_size() { | ||
| assert_eq!(std::mem::size_of::<RowGroupDecoderState>(), 232); | ||
| assert_eq!(std::mem::size_of::<RowGroupDecoderState>(), 256); |
There was a problem hiding this comment.
I think this is bigger because a RowSelection is now bigger.
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing export-mask (7b102f4) to 6ba533d (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing export-mask (7b102f4) to 6ba533d (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 |
|
🤔 looks like arrow_reader_row_filter is slightly slower. Rerunning to try an reproduce |
|
run benchmark arrow_reader_row_filter |
|
Thanks for your reviews @hhhizzz |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @haohuaijin and @hhhizzz -- I found this PR very easy to read and understand; It is also super well tested, and I think is a foundational piece for more efficient filter evaluation in parquet
cc @etseidl for your interest in all things parquet
cc @devanbenz as maybe some of your BMI work is relevant / could be used here
cc @Dandandan as we have discussed this before in the context of pushdown filter work (when the manipulation of RowSelections was expensive)
cc @adriangb as I think you are also interested in this
| if l.len() == r.len() { | ||
| return l | r; | ||
| } | ||
| let common = l.len().min(r.len()); |
There was a problem hiding this comment.
you might be able to do this more efficiently by copying the longer one and then using &= -- maybe in some follow on
|
Thanks for your review @alamb ❤️, i apply the suggestion, and filed follow-up issues to track some performance improve. and i also love to pick up and finish below performance improve.
The cc @hhhizzz |
|
Let;s go go go go! |
#10434) # Which issue does this PR close? - Closes #10424. # Rationale for this change Follow up to #10141. `selection/mod.rs` had grown past 2200 lines and mixed the `RowSelection` type 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.rs` goes from 2219 to 947 lines: | module | contents | | --- | --- | | `mod.rs` | `RowSelection`, `RowSelectionInner` and the public API | | `selector.rs` | run length backing: `RowSelector`, `RowSelectionIter`, and the `split_off` / `offset` / `limit` primitives | | `boolean.rs` | bitmap backing: `MaskSelection`, `MaskRunIter` and the mask primitives | | `algebra.rs` | `and_then`, `intersection` and `union` for both backings | | `ranges.rs` | page byte ranges and batch boundary expansion | | `cursor.rs` | `RowSelectionCursor`, `MaskCursor`, `SelectorsCursor`, `LoadedRowRanges`, `RowSelectionPolicy` / `RowSelectionStrategy` | # Are 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::selection` is identical before and after this PR, and the `split_off` / `trim` / `offset` / `limit` tests still go through the `RowSelection` methods 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 outside `selection/` needed modification. One internal note: `SelectorsCursor` and `MaskChunk` are no longer nameable outside `selection/`, as they moved into the private `cursor` module and are not re-exported. Neither was reachable from outside the crate before, since `selection` is `pub(crate)`.
|
I think we are very close to being able to implement something like this We probably already do it for unnested (leaf) nodes -- but need a bit more cleverness for structured nodes |
|
was this PR still considered an API change? i took a brief look and noticed:
i think both are these are considered breaking changes |
Hi,@Jefffrey , |
thanks i missed that
thoughts on if we should revert to the old signature (if possible) just to be on the safe side? i think only really niche use cases would be broken but maybe its good to keep flexibility if ever we need it 🤔 |
|
Yes, I agree that restoring the opaque return type would preserve the original abstraction boundary and give us more flexibility in the future. @haohuaijin, would you mind preparing a follow-up PR to restore the old cc @alamb for thoughts. |
sure, it is okay to restore the old |
|
#10450 pr is ready |
…0450) # Which issue does this PR close? - Follow-on to #10141, addressing #10141 (comment) # Rationale for this change #10141 changed `RowSelection::iter` from `impl Iterator<Item = &RowSelector>` to a named type, `RowSelectionIter<'_>`. As @Jefffrey pointed out, the opaque return type is worth keeping so we stay free to change the implementation later. # What changes are included in this PR? `RowSelection::iter` returns `impl Iterator<Item = &RowSelector>` again. Both match arms are already `std::slice::Iter<'_, RowSelector>`, so `RowSelectionIter` is not needed at all and is removed rather than made private. No behaviour change. # Are these changes tested? Covered by the existing `RowSelection` tests, which call `iter()` on both backings. `cargo test -p parquet --all-features` passes. # Are there any user-facing changes? No. `RowSelectionIter` was added in #10141 and never released, and `iter()` keeps the `Iterator<Item = &RowSelector>` contract callers already relied on.
|
Thanks for the catch @Jefffrey @haohuaijin and @hhhizzz |
6 participants
Which issue does this PR close?
RowSelectionto be backed by aBooleanBufferto reduce memory usage #10140Rationale for this change
RowSelectioncurrently stores selections asVec<RowSelector>(16 bytes per selector). This is compact for long runs, but expensive for scattered matches. With ~35% isolated single-row hits, it uses about 11.2 bytes per input row. ABooleanBufferuses 1 bit per input row, about 90x less memory.The reader can also choose the
Maskstrategy, which converts selectors back into a bitmap. When the caller already had a bitmap, this conversion round-trip is unnecessary.This PR lets
RowSelectionpreserve a caller-provided bitmap and pass it directly to mask execution.This is not intended to claim broad DataFusion / TPC-DS / ClickBench speedups. Current common DataFusion SQL paths generally do not naturally produce bitmap-backed
RowSelections. The practical benefit is for integrations that already have a row-level bitmap and need Parquet to consume it without materializing a large selector list.What changes are included in this PR?
RowSelectioncan now be backed by eitherVec<RowSelector>orBooleanBuffer. New public construction:Methods that can work directly on the bitmap now do so:
iter()still returns&RowSelector(non-breaking); mask-backed selections lazily materialize a selector cache on first call, while internal hot paths bypass it and use theBooleanBuffer/MaskRunIterdirectlyrow_count/skipped_row_countuse a cached popcountselects_anyusesset_indices().next()trimpreserves mask backing viaBooleanBuffer::sliceintersection/uniononMask+MaskuseBitAnd/BitOrsplit_offon a mask usesBooleanBuffer::slice(O(1), both halves stay mask-backed)limitslices at the selected-row boundary viafind_nth_set_bit_position, staying mask-backedoffsetfinds the first selected row to keep viafind_nth_set_bit_positionand rebuilds only the mask buffer, avoiding selector materializationand_thenapplies the inner selection over the mask's set positions, returning a mask-backed resultFromIterator<RowSelection>concatenatesBooleanBuffers when every input is mask-backedMixed inputs, and existing selector-backed inputs, still use the existing selector helpers. Existing callers keep the same behavior.
The reader (
ReadPlanBuilder::build) passes a mask-backed selection straight toRowSelectionCursor::new_mask_from_buffer, so it skips rebuilding the bitmap from selectors.Autoresolution works directly on the bitmap (early-exit run counting), without converting the backing.Integrates with #10288: both mask cursor constructors carry
LoadedRowRanges, so a caller-provided bitmap stays within loaded pages when page pruning skips pages.Also adds
MaskRunIter+RowSelection::as_maskfor zero-allocation RLE iteration over a mask, therow_selector_boolean_bufferbenchmark, andread_auto/ mask-backed input modes inrow_selection_cursor.Are these changes tested?
Yes. This PR extends the existing
RowSelectionunit tests with coverage for:BooleanBuffer, including empty and all-unset masksFrom<BooleanBuffer>split_off,limit,offset,and_then, and all-maskFromIterator<RowSelection>intersection/union, including uneven-length inputsfrom_filtersselector pathBooleanBuffer::slice(...), under bothMaskandAutopolicies (fix(parquet): support mask filtering across skipped pages #10288 integration seam)LoadedRowRangesboundariesboolean_mask_from_selectorsandtrim_mask(including non-zero offsets)cargo llvm-covAre there any user-facing changes?
No breaking API changes. New public APIs:
RowSelection::from_boolean_buffer,From<BooleanBuffer>,RowSelection::as_mask,MaskRunIter.