Skip to content

Use the null buffer's own offset when validating its length - #10709

Open
alliasgher wants to merge 1 commit into
apache:mainfrom
alliasgher:fix-null-buffer-offset-validation
Open

Use the null buffer's own offset when validating its length#10709
alliasgher wants to merge 1 commit into
apache:mainfrom
alliasgher:fix-null-buffer-offset-validation

Conversation

@alliasgher

Copy link
Copy Markdown

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:

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.

ArrayData::validate sized the null bitmap check with len_plus_offset, which
folds in ArrayData::offset. That offset does not apply to the null buffer,
as ArrayData::nulls documents, so a null buffer at offset 0 backing a sliced
array was rejected with 'null_bit_buffer size too small'.

Size the check from the null buffer's own offset and length instead.

The similar check in try_new is left alone: it takes a raw Buffer for which
the data offset does apply.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arrow Changes to the arrow crate arrow-data

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ArrayData offset (instead of null buffer offset) is used when validating nulls

1 participant