AndreaBozzo commented on code in PR #10916:
URL: https://github.com/apache/arrow-rs/pull/10916#discussion_r3889258395
##########
arrow-array/src/ffi.rs:
##########
@@ -485,12 +490,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
`len + offset` is at
+ // least 1 here (checked above), so the offset buffer has at
least two elements.
Review Comment:
Yeah, that was sloppy — I reused `len` for the byte length while writing
about the element count. Reworded in af93e15: the invariant is on `length` (the
element count from the top of `buffer_len`), which is `>= 1` here, so `len >= 2
* size_of::<i32>()` and the `- 1` can't underflow.
##########
arrow-array/src/ffi.rs:
##########
@@ -474,7 +474,12 @@ impl ImportedArrowArray<'_> {
length * (bits / 8)
}
(DataType::Utf8 | DataType::Binary, 2) => {
- if self.array.is_empty() {
+ // A zero-length array at offset 0 needs no offsets at all,
and the C Data
+ // Interface lets the producer pass a null pointer for a
buffer whose size
+ // would be 0, so the offset buffer must not be dereferenced
here. At a
Review Comment:
You're right, `length + 1` offsets are always required — my comment's
premise was wrong. The guard actually dates to #5964, which switched the values
length from `end - start` to `end`; what it protects is
`test_empty_string_with_non_zero_offset` (added in #5741), whose length-0 array
has a lone offset of `123` over an empty values buffer. That only holds at
offset 0, which is what this PR narrows it to.
Comment rewritten in af93e15 — happy to make any other changes you'd like.
--
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]
