alliasgher opened a new pull request, #10709:
URL: https://github.com/apache/arrow-rs/pull/10709
# Which issue does this PR close?
Closes #7379.
# Rationale for this change
`ArrayData::validate` sizes the null bitmap length check with
`len_plus_offset`, which folds in `ArrayData::offset`:
```rust
let actual_len = nulls.validity().len();
let needed_len = bit_util::ceil(len_plus_offset, 8);
```
That offset does not apply to the null buffer. `ArrayData::nulls` says so
directly: *"Note: `ArrayData::offset` does NOT apply to the returned
`NullBuffer`"*. The `NullBuffer` carries its own offset.
So a null buffer at offset 0 backing an array sliced to offset 50 is
rejected, using the reporter's test:
```
InvalidArgumentError("null_bit_buffer size too small. got 7 needed 13")
```
Decoupled offsets are a supported state rather than something the validator
was guarding against. `arrow-data/src/ffi.rs` `align_nulls` exists precisely
because `data.offset() != nulls.offset()` is legal: it fast-paths when they
match and re-aligns the bits otherwise.
As the issue notes, only the false-rejection direction is reachable today,
since there is currently no way to build an invalid `BooleanBuffer`. This is a
correctness fix to the check, not a soundness fix.
# What changes are included in this PR?
One line in `ArrayData::validate`, sizing the check from the null buffer's
own offset and length.
The near-identical check in `ArrayData::try_new` is deliberately left alone.
It takes a raw `null_bit_buffer: Option<Buffer>` for which the data offset
genuinely does apply, and it is pinned by `arrow/tests/array_validation.rs`
`test_bitmap_too_small`. I confirmed that test still passes.
# Are these changes tested?
Yes. Added `null_buffer_offset_is_independent_of_data_offset` in
`arrow-data/src/data.rs`, covering the reporter's scenario: 100 values sliced
to the last 50, then the same 50 nulls supplied both sliced (offset 50) and
unsliced (offset 0). Both must validate.
I checked it is not vacuous: reverting only the one-line fix while keeping
the test makes it fail with the exact error from the issue, `got 7 needed 13`.
Ran locally on `5ce0ebe`:
* `cargo test -p arrow-data`: 44 passed, plus 13 doctests
* `cargo test -p arrow-data -p arrow-array -p arrow-buffer -p arrow-select
-p arrow-cast`: all green
* `cargo test -p arrow --test array_validation`: 58 passed, including
`test_bitmap_too_small`
* `cargo fmt -p arrow-data -- --check` and `cargo clippy -p arrow-data
--all-targets -- -D warnings`: clean
One pre-existing failure unrelated to this change:
`util::test_util::tests::test_happy` needs the `testing/data` submodule, which
my clone did not initialize. It fails the same way on a pristine checkout.
# Are there any user-facing changes?
`ArrayData::validate` no longer rejects a valid null buffer whose offset
differs from the array's. No API change. No behavior change for null buffers
whose offset already matched the data offset.
# AI disclosure
Per `CONTRIBUTING.md` "AI Generated Submissions". I used an AI assistant to
help draft the fix, the test and this description. I reproduced the failure
with the reporter's verbatim test before changing anything, confirmed the
`try_new` site is a separate case that must not change and that its pinning
test still passes, and verified the new test fails with the fix reverted. I ran
every command listed above myself.
--
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]
