alliasgher commented on code in PR #10709:
URL: https://github.com/apache/arrow-rs/pull/10709#discussion_r3838405641
##########
arrow-data/src/data.rs:
##########
@@ -965,7 +965,8 @@ impl ArrayData {
}
let actual_len = nulls.validity().len();
- let needed_len = bit_util::ceil(len_plus_offset, 8);
+ // ArrayData::offset does not apply to the null buffer, which
carries its own offset
+ let needed_len = bit_util::ceil(nulls.offset() + nulls.len(), 8);
if actual_len < needed_len {
Review Comment:
Your instinct looks right, and it is `BooleanBuffer` that already guarantees
it. `BooleanBuffer::new` asserts exactly this invariant:
```rust
let total_len = bit_offset.saturating_add(bit_len);
assert!(total_len <= buffer.len() * 8, "buffer not large enough (...)");
```
`NullBuffer::new` wraps an already-constructed `BooleanBuffer`, and
`NullBuffer::new_unchecked` takes one too, so it can only make `null_count`
wrong, not the length. I could not find a way to reach `validate()` with a
`NullBuffer` whose backing buffer is too small.
Worth separating the two checks, because only one of them is dead. The
untrusted path is FFI, and that comes in as a raw `Buffer` through
`ArrayData::try_new`, which has its own `ceil(len_plus_offset, 8)` check at
`data.rs:336`. That one is reachable, returns an `Err` rather than panicking,
and is pinned by `test_bitmap_too_small`. This PR deliberately leaves it alone.
It is the `validate()` copy, operating on an already-constructed `NullBuffer`,
that cannot fire.
So removal loses nothing. I tried it: dropping the block builds clean,
`cargo clippy -p arrow-data --all-targets -- -D warnings` is clean
(`len_plus_offset` stays live, still used at lines 913 and 940), `cargo test -p
arrow-data` passes 44 plus 13 doctests, and `array_validation` still passes 58
including `test_bitmap_too_small`.
Happy either way, it is your call:
1. Remove the block. Then my regression test becomes a guard that a
decoupled-offset `NullBuffer` is accepted, which still has value.
2. Keep the fix as is, on the reasoning in the issue that a future
`BooleanBuffer` change could make it reachable again.
I lean slightly toward removal since the invariant has a clear owner, but I
do not have the history on why it ended up in `ArrayData`. Say the word and I
will push whichever.
--
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]
