AndreaBozzo opened a new issue, #10910: URL: https://github.com/apache/arrow-rs/issues/10910
### Describe the bug `ImportedArrowArray::buffer_len` returns `0` for the values buffer of a `Utf8` / `Binary` (and `LargeUtf8` / `LargeBinary`) array whenever `ArrowArray.length` is 0, without taking `ArrowArray.offset` into account: https://github.com/apache/arrow-rs/blob/4962d3842/arrow-array/src/ffi.rs#L476-L479 ```rust (DataType::Utf8 | DataType::Binary, 2) => { if self.array.is_empty() { return Ok(0); } ... ``` The offsets, however, are read *at* the array offset — `typed_offsets` slices from `self.offset`. So importing a zero-length array at a non-zero offset yields 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 when something validates the result: ``` Invalid argument error: First offset 1 of Utf8 is larger than values length 0 ``` This 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: passing such arrays to a Rust consumer that validates what it imports. ### To Reproduce `arrow-array/tests/zero_len_ffi.rs`, run with `cargo test -p arrow-array --features ffi --test zero_len_ffi`: ```rust use arrow_array::ffi::{from_ffi, to_ffi}; use arrow_buffer::Buffer; use arrow_data::ArrayData; use arrow_schema::DataType; #[test] fn zero_length_utf8_at_non_zero_offset() { // "x", "aa", "bb", viewed as zero elements starting at index 1. let offsets = Buffer::from_slice_ref([0i32, 1, 3, 5]); let values = Buffer::from("xaabb".as_bytes()); let data = ArrayData::try_new(DataType::Utf8, 0, None, 1, vec![offsets, values], vec![]) .expect("arrow-rs accepts this ArrayData in memory"); let (array, schema) = to_ffi(&data).unwrap(); let imported = unsafe { from_ffi(array, &schema) }.unwrap(); // from_ffi builds the ArrayData with new_unchecked, so the inconsistency // only surfaces when something validates. imported.validate_full().unwrap(); } ``` ``` thread 'zero_length_utf8_at_non_zero_offset' panicked at arrow-array/tests/zero_len_ffi.rs:19:30: called `Result::unwrap()` on an `Err` value: InvalidArgumentError("First offset 1 of Utf8 is larger than values length 0") ``` The array is built and validated by arrow-rs, exported by arrow-rs and imported by arrow-rs, so no other implementation is involved. Sweeping the offset shows the pattern is specific to `length == 0`: ``` offset 0, length 0 -> imported, len 0 offset 1, length 0 -> REJECTED: First offset 1 of Utf8 is larger than values length 0 offset 2, length 0 -> REJECTED: First offset 3 of Utf8 is larger than values length 0 offset 3, length 0 -> REJECTED: First offset 5 of Utf8 is larger than values length 0 offset 1, length 1 -> imported, len 1 offset 1, length 2 -> imported, len 2 offset 0, length 3 -> imported, len 3 ``` ### Expected behavior A round trip should preserve an array arrow-rs itself considers valid. The values buffer length should come from the last offset of the window, as it already does for non-empty arrays, rather than being short-circuited to 0 on length alone. ### Additional context Reproduced on `main` at 4962d3842. The same two arms are present in the 59.1.0 release (`arrow-array/src/ffi.rs:508`), so released versions are affected. The `is_empty()` guard was introduced in #5964 ("Fix FFI array offset handling"), which changed the values length from `end - start` to `end`. As far as I can tell the guard exists so that a length-0 array whose offsets buffer is legitimately absent is not dereferenced — the C Data Interface permits a null pointer for a buffer "if the size in bytes of the corresponding buffer would be 0". That reasoning only holds at offset 0: with a non-zero offset the producer must supply `length + offset + 1` offsets, so the buffer cannot be absent. If that reading is right, the smallest fix is to narrow the guard rather than remove it: ```diff (DataType::Utf8 | DataType::Binary, 2) => { - if self.array.is_empty() { + if self.array.is_empty() && self.array.offset() == 0 { return Ok(0); } @@ (DataType::LargeUtf8 | DataType::LargeBinary, 2) => { - if self.array.is_empty() { + if self.array.is_empty() && self.array.offset() == 0 { return Ok(0); } ``` With that change the reproducer above passes and `cargo test -p arrow-array --features ffi` is still green (770 unit tests, 205 doc-tests). I have not looked at whether the list types deserve the same treatment. If you agree with the diagnosis and the approach, I would be glad to open the PR. -- 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]
