Jefffrey commented on code in PR #10916:
URL: https://github.com/apache/arrow-rs/pull/10916#discussion_r3889332664
##########
arrow-array/src/ffi.rs:
##########
@@ -474,7 +474,15 @@ impl ImportedArrowArray<'_> {
length * (bits / 8)
}
(DataType::Utf8 | DataType::Binary, 2) => {
- if self.array.is_empty() {
+ // For a zero-length array at offset 0 the sole offset
describes no data,
+ // and producers do put an arbitrary value there -- see
+ // `test_empty_string_with_non_zero_offset`, whose lone offset
is 123 over
+ // an empty values buffer -- so it must not be used as a
length. Once the
+ // array offset is non-zero, the offsets up to and including
`offset`
+ // describe real preceding elements whose bytes the values
buffer still
+ // has to cover, so the length is read from the buffer as it
is for a
+ // non-empty array.
Review Comment:
```suggestion
// 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.
```
- avoid referring to a unit test, and can speak more generally
##########
arrow-array/src/ffi.rs:
##########
@@ -484,13 +492,15 @@ impl ImportedArrowArray<'_> {
// we assume that pointer is aligned for `i32`, as Utf8 uses
`i32` offsets.
#[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.
+ // Safety: `len` is the *byte* length of the offset buffer,
computed above as
+ // `(length + 1) * size_of::<i32>()`, so `len /
size_of::<i32>()` is its element
+ // count. Reaching here implies `length >= 1` (an empty array
at offset 0 returned
+ // above), hence at least two elements and no underflow on the
`- 1`.
(unsafe { *offset_buffer.add(len / size_of::<i32>() - 1) }) as
usize
Review Comment:
```suggestion
// 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 offset buffer
// is always non-empty.
```
- its confusing to state that `len` is calculated as `(length + 1) *
size_of::<i32>()` above when this isn't exactly accurate; i feel this way is
much simpler
--
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]
