This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new b54d812d6c arrow-data: remove unnecessary & incorrect null buffer
check in `ArrayData::validate` (#10709)
b54d812d6c is described below
commit b54d812d6c666f4a3a38bf6ae9fc2e5420cab480
Author: Ali Asghar <[email protected]>
AuthorDate: Sun Sep 6 17:38:14 2026 -0700
arrow-data: remove unnecessary & incorrect null buffer check in
`ArrayData::validate` (#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.
---------
Signed-off-by: Ali <[email protected]>
---
arrow-data/src/data.rs | 37 +++++++++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/arrow-data/src/data.rs b/arrow-data/src/data.rs
index 4b9acca77a..4c76842111 100644
--- a/arrow-data/src/data.rs
+++ b/arrow-data/src/data.rs
@@ -1027,14 +1027,6 @@ impl ArrayData {
)));
}
- let actual_len = nulls.validity().len();
- let needed_len = bit_util::ceil(len_plus_offset, 8);
- if actual_len < needed_len {
- return Err(ArrowError::InvalidArgumentError(format!(
- "null_bit_buffer size too small. got {actual_len} needed
{needed_len}",
- )));
- }
-
if nulls.len() != self.len {
return Err(ArrowError::InvalidArgumentError(format!(
"null buffer incorrect size. got {} expected {}",
@@ -3575,6 +3567,35 @@ mod tests {
ArrayData::new_null(&dt, 1).validate_full().unwrap();
}
+ #[test]
+ fn null_buffer_offset_is_independent_of_data_offset() {
+ // 100 values sliced down to the last 50, so the data has offset 50.
+ let int_data = ArrayData::builder(DataType::UInt32)
+ .offset(50)
+ .len(50)
+ .add_buffer(Buffer::from_vec(vec![0_u32; 100]))
+ .build()
+ .unwrap();
+ int_data.validate().unwrap();
+
+ // A null buffer that happens to share the data's offset.
+ let nulls = NullBuffer::new(BooleanBuffer::from(vec![false;
100]).slice(0, 50));
+ let with_sliced_nulls = int_data
+ .clone()
+ .into_builder()
+ .nulls(Some(nulls))
+ .build()
+ .unwrap();
+ with_sliced_nulls.validate().unwrap();
+
+ // The same 50 nulls at offset 0. ArrayData::offset does not apply to
the
+ // null buffer, so this is just as valid and must not be rejected.
+ let nulls = NullBuffer::new(BooleanBuffer::from(vec![false; 50]));
+ let with_unsliced_nulls =
int_data.into_builder().nulls(Some(nulls)).build().unwrap();
+ with_unsliced_nulls.validate().unwrap();
+ assert_eq!(with_unsliced_nulls.null_count(), 50);
+ }
+
fn test_both_builder_and_array_data(
data_type: DataType,
len: usize,
