AndreaBozzo commented on code in PR #10916:
URL: https://github.com/apache/arrow-rs/pull/10916#discussion_r3889258444
##########
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.
I refreshed the pr body as it didn't match reality after latest commit,
thanks for your input @Jefffrey !
Comment rewritten in af93e15 — happy to make any other changes you'd like.
##########
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.
I refreshed the pr body as it didn't match reality after latest commit,
thanks for your input @Jefffrey !
--
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]
