Jefffrey commented on code in PR #10916:
URL: https://github.com/apache/arrow-rs/pull/10916#discussion_r3889219513
##########
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:
i thought offset buffers must always have `length + 1` elements?
##########
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:
bit confused here since `len` is different from array length; `len` is byte
length and it being at least 1 doesn't technically guarantee safety since if
`len = 1` then `len / size_of::<i32>()` is `0`
--
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]
