WIP: Boolean mask backed RowSelection - #8902
Closed
alamb wants to merge 14 commits into
Closed
Conversation
This was referenced Nov 21, 2025
alamb
commented
Nov 25, 2025
| /// Optimized version of `boolean_buffer_and_then` using BMI2 PDEP instructions. | ||
| /// This function performs the same operation but uses bit manipulation instructions | ||
| /// for better performance on supported x86_64 CPUs. | ||
| pub fn boolean_buffer_and_then(left: &BooleanBuffer, right: &BooleanBuffer) -> BooleanBuffer { |
Contributor
Author
There was a problem hiding this comment.
@XiangpengHao did this it in liquid cache:
I will copy that new version over here in this PR
alamb
force-pushed
the
alamb/sketch_boolean_selection
branch
from
December 1, 2025 18:48
826e01f to
e4b2283
Compare
rluvaton
reviewed
Dec 1, 2025
rluvaton
reviewed
Dec 1, 2025
Comment on lines
+37
to
+39
| let mut buffer = MutableBuffer::from_len_zeroed(left.values().len()); | ||
| buffer.copy_from_slice(left.values()); | ||
| let mut builder = BooleanBufferBuilder::new_from_buffer(buffer, left.len()); |
Member
There was a problem hiding this comment.
Note that you have a bug here and also copy more data than needed.
BooleanBuffer have an offset to start from the values and here you:
- add more data than needed by creating a buffer with everything
- the left set_indices here need to be offseted by the
left.offset()in order to set the correct bit (you should fix to not copy unneeded values and then you can keep this)
Contributor
Author
There was a problem hiding this comment.
This was a really good call. When I was porting over the tests they caught exactly this problem
I actually found we could do the same thing, but probably faster via
let mut builder = BooleanBufferBuilder::new(left.len());
builder.append_buffer(&left);
Which then calls BooleanBufferBuilder::append_packed_range
(conveniently I know someone who just optimized that function 😆 )
rluvaton
reviewed
Dec 1, 2025
|
|
||
| if left.len() == right.len() { | ||
| debug_assert_eq!(left.count_set_bits(), left.len()); | ||
| return right.clone(); |
Contributor
Author
There was a problem hiding this comment.
I think in this case left is always (and asserted) to be all 1s so no AND is necessary
rluvaton
reviewed
Dec 1, 2025
Comment on lines
+43
to
+49
| for bit_idx in left.set_indices() { | ||
| let predicate = other_bits | ||
| .next() | ||
| .expect("Mismatch in set bits between self and other"); | ||
| if !predicate { | ||
| builder.set_bit(bit_idx, false); | ||
| } |
Member
There was a problem hiding this comment.
maybe a faster approach would be to use set slices and then use AND with right starting from the last location
alamb
force-pushed
the
alamb/sketch_boolean_selection
branch
4 times, most recently
from
December 5, 2025 22:13
f767c2e to
4e01ca4
Compare
alamb
force-pushed
the
alamb/sketch_boolean_selection
branch
from
December 12, 2025 13:51
d312a8c to
8d9b155
Compare
Contributor
Author
|
run benchmark arrow_reader_clickbench |
|
🤖 |
Contributor
Author
|
show benchmark queue |
|
🤖 Hi @alamb, you asked to view the benchmark queue (#8902 (comment)).
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
🤔 going the wrong direction |
Contributor
Author
|
run benchmark arrow_reader_clickbench |
Contributor
Author
|
run benchmark arrow_reader |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
I have been profiling this morning, and basically I have concluded I need to do two things: #8844 (comment) |
Contributor
Author
|
I don't really have time to push this forward now |
alamb
added a commit
that referenced
this pull request
Jul 24, 2026
) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Closes #10140 - closes #8844 - closes #6624 from @XiangpengHao - Past attempts: #8902 # Rationale for this change `RowSelection` currently stores selections as `Vec<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. A `BooleanBuffer` uses 1 bit per input row, about 90x less memory. The reader can also choose the `Mask` strategy, which converts selectors back into a bitmap. When the caller already had a bitmap, this conversion round-trip is unnecessary. **This PR lets `RowSelection` preserve 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 `RowSelection`s. 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? `RowSelection` can now be backed by either `Vec<RowSelector>` or `BooleanBuffer`. New public construction: ```rust pub fn RowSelection::from_boolean_buffer(mask: BooleanBuffer) -> Self; impl From<BooleanBuffer> for RowSelection; ``` 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 the `BooleanBuffer` / `MaskRunIter` directly - `row_count` / `skipped_row_count` use a cached popcount - `selects_any` uses `set_indices().next()` - `trim` preserves mask backing via `BooleanBuffer::slice` - `intersection` / `union` on `Mask`+`Mask` use `BitAnd` / `BitOr` - `split_off` on a mask uses `BooleanBuffer::slice` (`O(1)`, both halves stay mask-backed) - `limit` slices at the selected-row boundary via `find_nth_set_bit_position`, staying mask-backed - `offset` finds the first selected row to keep via `find_nth_set_bit_position` and rebuilds only the mask buffer, avoiding selector materialization - `and_then` applies the inner selection over the mask's set positions, returning a mask-backed result - `FromIterator<RowSelection>` concatenates `BooleanBuffer`s when every input is mask-backed Mixed 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 to `RowSelectionCursor::new_mask_from_buffer`, so it skips rebuilding the bitmap from selectors.** `Auto` resolution 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_mask` for zero-allocation RLE iteration over a mask, the `row_selector_boolean_buffer` benchmark, and `read_auto` / mask-backed input modes in `row_selection_cursor`. # Are these changes tested? Yes. This PR extends the existing `RowSelection` unit tests with coverage for: - constructing from `BooleanBuffer`, including empty and all-unset masks - `From<BooleanBuffer>` - preserving mask backing across clone, `split_off`, `limit`, `offset`, `and_then`, and all-mask `FromIterator<RowSelection>` - falling back to selector backing for mixed-backed concatenation - equality between equivalent selector-backed and mask-backed selections - mask-backed `intersection` / `union`, including uneven-length inputs - fuzz-style equivalence between mask-backed selections and the existing `from_filters` selector path - sparse-page regression tests from a non-byte-aligned `BooleanBuffer::slice(...)`, under both `Mask` and `Auto` policies (#10288 integration seam) - mask chunks stop at `LoadedRowRanges` boundaries - fuzz tests for `boolean_mask_from_selectors` and `trim_mask` (including non-zero offsets) - remaining branches checked with `cargo llvm-cov` # Are there any user-facing changes? No breaking API changes. New public APIs: `RowSelection::from_boolean_buffer`, `From<BooleanBuffer>`, `RowSelection::as_mask`, `MaskRunIter`. --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
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?
TODOs
Rationale for this change
Make the parquet predicate evaluation faster by not converting back/forth between BooleanArray and RowSelection as much
What changes are included in this PR?
Are these changes tested?
TBD
Are there any user-facing changes?
Internal notes for myself