haohuaijin opened a new pull request, #10444:
URL: https://github.com/apache/arrow-rs/pull/10444
# Which issue does this PR close?
No dedicated issue — this was found while implementing #10425, whose
optimization is the first caller that depends on the behaviour fixed here.
Happy to file one if you'd prefer to track it separately.
# Rationale for this change
`apply_bitwise_binary_op` and `apply_bitwise_unary_op` are built around the
invariant that only the bits in `offset_in_bits..offset_in_bits + len_in_bits`
are modified. That intent is stated in the doc comments of the helpers
implementing it — `align_to_byte` ("while preserving all other bits in the
byte"), `set_remainder_bits` ("leaving all other bits in the affected bytes
unchanged") and `handle_mutable_buffer_remainder_unary` ("ensuring that bits
outside the operation range are preserved") — but two paths did not honour it.
**1. `align_to_byte` ignored `len_in_bits`.** It wrote every bit from
`bit_offset` up to the byte boundary. When the requested range both started and
ended inside the same non-byte-aligned byte, the trailing bits of that byte
were overwritten with `op` applied to padding:
```rust
let mut left = vec![0b11111111u8, 0b11111111u8];
let right = vec![0b00000000u8, 0b00000000u8];
// AND a single bit at offset 1
apply_bitwise_binary_op(&mut left, 1, &right, 0, 1, |a, b| a & b);
// expected [0b11111101, 0b11111111], got [0b00000001, 0b11111111]
```
**2. `set_remainder_bits` dropped the boundary byte's out-of-range bits.**
It read that byte into the *low* bits of a `u64` and masked it with `!((1 <<
remainder_len) - 1)`. Whenever the remainder spanned more than one byte the
mask selected nothing, so those bits were zeroed rather than preserved. This
affected any call whose length left a remainder of 9..63 bits that is not a
multiple of 8.
## No existing caller was affected
I checked all four in-tree callers, which is why this went unnoticed:
| Caller | Why it is immune |
| --- | --- |
| `BooleanBufferBuilder::append_packed_range` | Copies with `\|_a, b\| b`
into capacity just zeroed by `advance()`; both bugs force out-of-range bits to
`0`, which is what they already were |
| `BooleanBuffer::bitwise_bin_op_assign` | Runs only on a uniquely owned
buffer (`into_mutable()`) and re-wraps the result with the original
offset/length, so out-of-range bits are unobservable |
| `BooleanArray::try_bitwise_unary_in_place` | Same uniquely-owned pattern |
| `BooleanArray::try_bitwise_bin_op_in_place` | Same uniquely-owned pattern |
# What changes are included in this PR?
- `align_to_byte` takes `len_in_bits` and masks its write to
`bit_offset..bit_offset + min(8 - bit_offset, len_in_bits)`.
- `set_remainder_bits` shifts the boundary byte to the position it actually
occupies within the word before masking.
- The guarantee is now stated on both public functions, since callers that
read the surrounding bits back rely on it.
# Are these changes tested?
Yes. The root cause of the gap was that the two shared test helpers only
asserted the bits *inside* the operated range, so out-of-range corruption was
invisible to every test using them. Both helpers now also assert that every bit
outside the range is byte-for-byte identical before and after, which
retroactively covers all of their existing call sites — that alone surfaced bug
2 in 11 pre-existing tests.
Added on top of that:
- `test_ops_ending_inside_the_first_partial_byte` — sweeps every
`offset`/`len` pair contained in one partial byte, for AND/OR/XOR and NOT,
including differing left/right offsets.
- `test_and_within_first_partial_byte_preserves_trailing_bits` and
`test_not_within_first_partial_byte_preserves_trailing_bits` — minimal
regression tests for the two reproducers.
`cargo test -p arrow-buffer` passes (342 unit + 54 doc tests), as do
`arrow-array`, `arrow` and `arrow-select`, which exercise the fixed helpers
through the callers above. Clippy and fmt are 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
API signature changes and no breaking changes — as shown above, no existing
caller could observe the old behaviour. The preservation guarantee is now
explicit in the rustdoc for both functions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
