perf(parquet): single-allocation intersect_masks/union_masks for unequal lengths#10446
Draft
haohuaijin wants to merge 3 commits into
Draft
perf(parquet): single-allocation intersect_masks/union_masks for unequal lengths#10446haohuaijin wants to merge 3 commits into
haohuaijin wants to merge 3 commits into
Conversation
…ace bitwise ops `apply_bitwise_binary_op` and `apply_bitwise_unary_op` are documented internally as modifying only the bits in `offset_in_bits..offset_in_bits + len_in_bits` -- see the doc comments on `align_to_byte`, `set_remainder_bits` and `handle_mutable_buffer_remainder_unary`. Two paths did not honour that invariant: * `align_to_byte` wrote every bit from `bit_offset` to the byte boundary, ignoring `len_in_bits`. When the requested range started and ended inside the same non-byte-aligned byte, the trailing bits of that byte were overwritten with the result of `op` applied to padding. For example `apply_bitwise_binary_op(left, 1, right, 0, 1, |a, b| a & b)` cleared bits 2..8. * `set_remainder_bits` read the boundary byte into the low bits of a `u64` and masked it with `!((1 << remainder_len) - 1)`. Whenever the remainder spanned more than one byte that mask selected nothing, so the out-of-range bits of the boundary byte were dropped to zero instead of being preserved. No existing caller was affected. `BooleanBufferBuilder::append_packed_range` copies with `|_a, b| b` into freshly zeroed capacity, and the `BooleanBuffer`/`BooleanArray` in-place paths only run on uniquely owned buffers and re-wrap the result with the original offset and length, so the out-of-range bits were either already zero or unobservable. The existing tests only asserted the bits inside the operated range. The two shared test helpers now also assert that every bit outside the range is identical before and after, which covers all of their call sites, plus targeted tests for ranges contained in a single partial byte. The guarantee is now stated on both public functions, since callers that read the surrounding bits back depend on it.
github-actions
Bot
added
parquet
…vation - Rename `align_to_byte`'s new parameter to `remaining_len_in_bits`; the binary caller passes the already-clamped `bits_to_next_byte` while the unary caller passes the full length, so "total bits to process" only described one of them. - Add `debug_assert_ne!(bit_offset, 0)` to `align_to_byte`, making the documented "not byte-aligned" precondition checked, and drop the dead `& 0xFF` from the write mask now that the bound is explicit. - Use the slice length instead of recomputing `ceil(remainder_len, 8)` in `set_remainder_bits`; the assertion above already proves them equal. - Cover `len == 8 - offset` in `test_ops_ending_inside_the_first_partial_byte`; the exclusive range skipped the byte-boundary case and never ran at all for `offset == 7`. - Add targeted regression tests for the multi-byte remainder bug. It was only covered indirectly through the shared helpers, so weakening them would have let it regress silently.
…ual lengths
When two mask-backed `RowSelection`s have different lengths, the bitwise
`intersection`/`union` path allocated twice: once for the bitwise result over
the common prefix, and once more for a `BooleanBufferBuilder` that appended
the prefix and then the longer side's tail, copying both.
Copy the longer mask once into a `MutableBuffer` instead and apply `&=`/`|=`
in place over the common prefix with `bit_util::apply_bitwise_binary_op`. The
tail is already in the right place, so it needs no copy at all.
Neither the mask offsets nor the prefix length are assumed to be byte aligned.
Masks reaching this path may carry a non-zero bit offset from
`BooleanBuffer::slice`, and the prefix may end mid byte. The copy keeps the
longer mask's sub-byte offset rather than re-aligning it, so it stays a plain
byte copy, and that offset is carried over to the returned buffer.
Criterion, mask-backed `RowSelection::intersection`/`union`, ~1/3 density:
before after change
intersect 3M / 2M 16.60 us 12.96 us -21.9%
union 3M / 2M 16.57 us 13.12 us -20.8%
intersect 3M / 3M-1 20.22 us 16.51 us -18.3%
union 3M / 3M-1 20.19 us 16.66 us -17.5%
intersect 100K / 1K 509 ns 278 ns -45.4%
union 100K / 1K 525 ns 270 ns -48.6%
The gain grows with the tail, which the old code copied through the builder
for no reason.
Tested by an exhaustive sweep over unaligned offset and length combinations
and a randomized fuzz test, both checked against a bit-by-bit reference.
Closes apache#10425
haohuaijin
force-pushed
the
perf-uneven-mask-intersect-union
branch
from
July 27, 2026 04:56
d332086 to
236925a
Compare
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
1 participant
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?
intersect_masks/union_masksfor unequal lengths with in-place bitwise ops #10425.Important
Draft: stacked on #10444. This branch is based on that PR, so the diff here currently shows both commits. The optimization depends on the fix in #10444 — without it the mask tail gets corrupted and the tests below fail. Once #10444 merges I'll rebase onto
mainand this reduces to the singlealgebra.rscommit.Rationale for this change
When two mask-backed
RowSelections have different lengths,intersect_masks/union_masksallocate twice: once for the bitwise result over the common prefix, then again for aBooleanBufferBuilderthat appends the prefix and the longer side's tail — copying both.What changes are included in this PR?
Copy the longer mask once into a
MutableBufferand apply&=/|=in place over the common prefix viabit_util::apply_bitwise_binary_op. The tail is already in the right place, so it needs no copy at all.Neither the mask offsets nor the prefix length are assumed to be byte aligned — masks reaching this path may carry a non-zero bit offset from
BooleanBuffer::slice, and the prefix may end mid byte. The copy keeps the longer mask's sub-byte offset instead of re-aligning it, so it stays a plain byte copy, and that offset is carried over to the result.Are these changes tested?
Yes —
test_uneven_masks_with_offsetssweeps offsets{0,1,5,8,13,64,67}×{0,1,3,8,60,64,70}against several length pairs, andtest_uneven_masks_fuzzruns 200 randomized rounds. Both compare against a bit-by-bit reference implementation.cargo test -p parquet --all-featurespasses (1284 tests); clippy and fmt clean.Criterion over mask-backed
RowSelection::intersection/unionat ~1/3 density:Measured with an ad-hoc bench harness rather than a committed one, since there's no existing bench covering mask-backed selections —
row_selector.rsbuilds its inputs withfrom_filters, which is selector-backed and doesn't reach this path. Happy to add a permanent bench if you'd like one.The gain grows with the tail, which the old code copied through the builder for no reason.
Are there any user-facing changes?
No API changes.
intersect_masks/union_masksarepub(super); the observable behaviour ofRowSelection::intersection/unionis unchanged.