fix(arrow-buffer): preserve bits outside the requested range in in-place bitwise ops - #10444
Open
haohuaijin wants to merge 3 commits into
Open
fix(arrow-buffer): preserve bits outside the requested range in in-place bitwise ops#10444haohuaijin 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.
…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.
Contributor
Author
|
While implementing #10425 I hit corrupted mask tails, and traced it to two bugs in the in-place bitwise helpers — both write outside
cc @alamb |
Contributor
|
run benchmark builder |
This comment was marked as duplicate.
This comment was marked as duplicate.
Contributor
|
run benchmark builder |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
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?
No dedicated issue — found while implementing #10425, whose optimization is the first caller to depend on the behaviour fixed here. Happy to file one if you'd prefer it tracked separately.
Rationale for this change
apply_bitwise_binary_opandapply_bitwise_unary_opare built around the invariant that only the bits inoffset_in_bits..offset_in_bits + len_in_bitsare modified — the helpers implementing it (align_to_byte,set_remainder_bits,handle_mutable_buffer_remainder_unary) all say so in their doc comments. Two paths didn't honour it:1.
align_to_byteignoredlen_in_bits, writing every bit frombit_offsetto the byte boundary. If the range started and ended inside the same non-byte-aligned byte, the trailing bits were overwritten withopapplied to padding:2.
set_remainder_bitszeroed the boundary byte's out-of-range bits, because it read that byte into the low bits of au64and masked it with!((1 << remainder_len) - 1). Once the remainder spanned more than one byte, the mask selected nothing. This hit any length leaving a remainder of 9..63 bits that isn't a multiple of 8.No existing caller was affected, which is why it went unnoticed:
BooleanBufferBuilder::append_packed_rangecopies with|_a, b| binto capacityadvance()just zeroed, and theBooleanBuffer/BooleanArrayin-place paths run only on uniquely owned buffers and re-wrap with the original offset/length. So the out-of-range bits were either already zero or unobservable.What changes are included in this PR?
align_to_bytetakeslen_in_bitsand masks its write tobit_offset..bit_offset + min(8 - bit_offset, len_in_bits).set_remainder_bitsshifts the boundary byte to the position it actually occupies in the word before masking.Are these changes tested?
Yes. The gap existed because the two shared test helpers only asserted bits inside the operated range. They now also assert every bit outside it is byte-for-byte identical before and after, which retroactively covers all their call sites — that alone caught bug 2 in 11 pre-existing tests. Added
test_ops_ending_inside_the_first_partial_byte(sweeps every offset/len pair inside one partial byte, for AND/OR/XOR and NOT) plus minimal regression tests for the two reproducers.cargo test -p arrow-bufferpasses (342 unit + 54 doc), as doarrow-array,arrowandarrow-select. Clippy and fmt clean.Are there any user-facing changes?
A behaviour change in two public functions, in the direction of the documented intent: bits outside the requested range are no longer clobbered. No signature changes, and no existing caller could observe the old behaviour. The guarantee is now explicit in the rustdoc.