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 70fa5bcf21 Fix FFI import of zero-length `Utf8`/`Binary` arrays at a
non-zero offset (#10916)
70fa5bcf21 is described below
commit 70fa5bcf21924c5a9ea34ee2df846ffae43f2bc6
Author: Andrea Bozzo <[email protected]>
AuthorDate: Sun Aug 30 23:49:40 2026 +0200
Fix FFI import of zero-length `Utf8`/`Binary` arrays at a non-zero offset
(#10916)
> **Note (edited after review):** this description originally justified
the
> `is_empty()` guard by the C Data Interface's allowance for a null
pointer on a
> zero-sized buffer. That was wrong — the columnar format requires
`length + 1`
> offsets, so an empty array's offsets buffer is one element, not 0
bytes. The
> guard actually dates to #5964 and exists to protect
> `test_empty_string_with_non_zero_offset` (added in #5741), whose
length-0 array
> carries a lone offset of `123` over an empty values buffer. Thanks to
@Jefffrey
> for catching it. Corrected below and in the code comment (af93e15);
the change
> itself is unaffected.
# 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 because
the sole
offset of a zero-length array is unconstrained and producers put
arbitrary values
there — see the existing `test_empty_string_with_non_zero_offset` (added
in #5741),
whose lone offset is `123` over an empty values buffer, and which `end`
alone would
size at 123 bytes. That reasoning only holds at offset 0: once the
offset is
non-zero, the offsets up to and including `offset` describe real
preceding elements
whose bytes the values buffer must still cover.
# 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.
---
arrow-array/src/ffi.rs | 65 ++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 61 insertions(+), 4 deletions(-)
diff --git a/arrow-array/src/ffi.rs b/arrow-array/src/ffi.rs
index 1957031c22..14235631a8 100644
--- a/arrow-array/src/ffi.rs
+++ b/arrow-array/src/ffi.rs
@@ -474,7 +474,15 @@ impl ImportedArrowArray<'_> {
length * (bits / 8)
}
(DataType::Utf8 | DataType::Binary, 2) => {
- if self.array.is_empty() {
+ // We can short circuit for empty arrays with offset 0 since
we know
+ // the values buffer must also be empty, and the single offset
present
+ // in the offsets buffer can be an arbitrary value from the
producer.
+ //
+ // If the array is empty yet has a non-zero offset, the C data
interface
+ // guarantees there are `length + offset` values encoded in
the buffer,
+ // so we must find the real size of the values buffer from the
offsets
+ // buffer.
+ if self.array.is_empty() && self.array.offset() == 0 {
return Ok(0);
}
@@ -485,12 +493,13 @@ impl ImportedArrowArray<'_> {
#[expect(clippy::cast_ptr_alignment)]
let offset_buffer = self.array.buffer(1).cast::<i32>();
// Safety: `len` is the byte length of the offset buffer;
dividing by `size_of::<i32>()`
- // gives the number of i32 elements. The `- 1` is safe because
the array is non-empty
- // (checked above), so the offset buffer has at least one
element.
+ // gives the number of i32 elements. The `- 1` is safe because
the offset buffer
+ // is always non-empty.
(unsafe { *offset_buffer.add(len / size_of::<i32>() - 1) }) as
usize
}
(DataType::LargeUtf8 | DataType::LargeBinary, 2) => {
- if self.array.is_empty() {
+ // See the note on the `Utf8` / `Binary` arm above.
+ if self.array.is_empty() && self.array.offset() == 0 {
return Ok(0);
}
@@ -1695,6 +1704,54 @@ mod tests_from_ffi {
test_round_trip(&imported_array.consume()?)
}
+ /// A zero-length `Utf8` / `Binary` array at a non-zero offset must
survive a
+ /// round trip: the length of the values buffer has to come from the last
offset
+ /// of the window, as it already does for non-empty arrays, rather than
being
+ /// short-circuited to 0 on the length alone.
+ ///
+ /// <https://github.com/apache/arrow-rs/issues/10910>
+ #[test]
+ fn test_zero_length_bytes_at_non_zero_offset() -> Result<()> {
+ // "x", "aa", "bb", viewed as zero elements starting at `offset`.
+ let small = Buffer::from_slice_ref([0i32, 1, 3, 5]);
+ let large = Buffer::from_slice_ref([0i64, 1, 3, 5]);
+ let values = Buffer::from(b"xaabb".as_slice());
+
+ for (data_type, offsets) in [
+ (DataType::Utf8, &small),
+ (DataType::Binary, &small),
+ (DataType::LargeUtf8, &large),
+ (DataType::LargeBinary, &large),
+ ] {
+ for offset in 0..4 {
+ let data = ArrayData::try_new(
+ data_type.clone(),
+ 0,
+ None,
+ offset,
+ vec![offsets.clone(), values.clone()],
+ vec![],
+ )?;
+
+ let array = FFI_ArrowArray::new(&data);
+ let schema = FFI_ArrowSchema::try_from(&data_type)?;
+ let imported = unsafe { from_ffi(array, &schema) }?;
+
+ // `from_ffi` builds the `ArrayData` with `new_unchecked`, so
an
+ // inconsistency only surfaces once something validates it.
+ imported.validate_full()?;
+ assert_eq!(imported.len(), 0);
+ assert_eq!(imported, data);
+
+ // The values buffer is sized from the last offset of the
window,
+ // not short-circuited to 0.
+ assert_eq!(imported.buffers()[1].len(), [0, 1, 3, 5][offset]);
+ }
+ }
+
+ Ok(())
+ }
+
fn roundtrip_string_array(array: StringArray) -> StringArray {
let data = array.into_data();
