AndreaBozzo opened a new pull request, #10916:
URL: https://github.com/apache/arrow-rs/pull/10916
# Which issue does this PR close?
Closes #10910.
# Rationale for this change
`ImportedArrowArray::buffer_len` short-circuited the values buffer (buffer
2) of
`Utf8`/`Binary` and `LargeUtf8`/`LargeBinary` arrays to `0` whenever
`ArrowArray.length` was `0`, without taking `ArrowArray.offset` into
account. The
offsets, however, are read *at* the array offset, so importing a zero-length
array
at a non-zero offset produced an `ArrayData` whose values buffer is shorter
than
its own offsets describe. `from_ffi` builds it with `new_unchecked`, so
nothing is
reported at import time; the inconsistency only surfaces once something
validates
the result:
```
Invalid argument error: First offset 1 of Utf8 is larger than values length 0
```
The shape is not reachable through arrow-rs's own slicing — `slice(n, 0)`
normalises
the offset back to 0 — so it takes a producer on the other side of the
interface.
pyarrow produces one readily with `pa.array(["x", "aa", "bb"]).slice(1, 0)`,
and that
is how this was found.
The `is_empty()` guard came in with #5964 ("Fix FFI array offset handling"),
which
changed the values length from `end - start` to `end`. It exists so that a
length-0
array whose offsets buffer carries no meaningful last offset is not sized
from it —
the C Data Interface permits a null pointer for a buffer whose size in bytes
would
be 0, and producers also emit arbitrary trailing offsets for empty arrays
(see the
existing `test_empty_string_with_non_zero_offset`, whose lone offset is
`123`). That
reasoning only holds at offset 0: with a non-zero offset the producer must
supply
`offset + length + 1` offsets, so the buffer is present and its window has a
real
last offset.
# What changes are included in this PR?
Narrow the guard to `offset == 0` in both arms, so a zero-length array at a
non-zero
offset takes the same path as a non-empty one and reads its values length
from the
last offset of the window.
The stale safety comment above the `- 1` is corrected too: it claimed the
array is
non-empty, whereas the invariant that makes the index sound is `len + offset
>= 1`.
The list types are untouched: for `List`/`LargeList`/`Map`, buffer 2 is
child data
rather than a buffer, so `buffer_len` never reaches these arms for them.
# Are these changes tested?
Yes — `tests_from_ffi::test_zero_length_bytes_at_non_zero_offset` sweeps
offsets
`0..4` over `Utf8`, `Binary`, `LargeUtf8` and `LargeBinary`, asserting the
round trip
passes `validate_full`, matches the exported `ArrayData`, and sizes the
values buffer
from the window's last offset.
I checked that it fails without the fix, and that it discriminates the two
arms
independently: reverting only the `i32` arm fails on `Utf8`, reverting only
the `i64`
arm fails on `LargeUtf8`, both with the error above. It does not separately
discriminate `Binary` from `Utf8` (the loop stops at the first failure), but
each pair
shares a single match arm, so there is no distinct code path uncovered.
`cargo test -p arrow-array --features ffi` is green (771 unit + 205 doc
tests), as is
`--features ffi,force_validate`; `cargo fmt --check` and `cargo clippy
--all-targets`
are clean.
One thing I looked at while reviewing: whether widening the dereference can
now be
reached with a null offsets pointer. It cannot. `buffers()` resolves buffer
1 before
buffer 2 (`map` is lazy and `collect::<Result<_>>` short-circuits), and
`buffer_len(1)`
is `(length + 1) * width`, never 0 — so a null offsets pointer always fails
at index 1
with `The external buffer at position 1 is null.` before `buffer_len(2)` is
called.
# Are there any user-facing changes?
No API changes. Zero-length `Utf8`/`Binary`/`LargeUtf8`/`LargeBinary` arrays
imported
at a non-zero offset now yield a valid `ArrayData` instead of one that fails
validation. There is no breaking change.
--
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]
