fix: Adjust FFI_ArrowArray offset based on the offset of offset buffer#5895
fix: Adjust FFI_ArrowArray offset based on the offset of offset buffer#5895viirya merged 4 commits intoapache:masterfrom
Conversation
viirya
force-pushed
the
fix_string_offset_ffi
branch
from
60c3e6c to
eb51190
Compare
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @viirya -- I verified that this PR fixes the test case and therefore I think it is an improvement.
However, I am a bit confused about how the pointer and data buffer can be different so I left some additional comments / suggestions on how we might make that better
| } | ||
|
|
||
| #[test] | ||
| fn test_extend_imported_string_slice() { |
There was a problem hiding this comment.
I ran this test without the code changes in this PR and it fails like
range end index 10890 out of range for slice of length 5500
thread 'ffi::tests_from_ffi::test_extend_imported_string_slice' panicked at arrow-data/src/transform/variable_size.rs:38:29:
range end index 10890 out of range for slice of length 5500
stack backtrace:Thus I believe the test correctly covers the new code
arrow-array/src/ffi.rs
Outdated
| test_round_trip(&imported_array.consume()?) | ||
| } | ||
|
|
||
| fn export_string(array: StringArray) -> StringArray { |
There was a problem hiding this comment.
Perhaps calling this function "roundtrip_string_array" would better describe what it does
arrow-data/src/ffi.rs
Outdated
| // the `FFI_ArrowArray` offset field. | ||
| Some(unsafe { | ||
| let b = &data.buffers()[0]; | ||
| b.as_ptr().offset_from(b.get_bytes().ptr().as_ptr()) as usize |
There was a problem hiding this comment.
I think this calculation assumes that the string data begins at the start of the buffer (aka b.as_ptr() is the base).
I double checked and the comments seem to imply that that is always the case
https://github.com/apache/arrow-rs/blob/c096172b769da8003ea7816086df60f38229a891/arrow-buffer/src/bytes.rs#L40-L39
Since this is calculating on internal state of Buffer, I think this code would be easier to understand / document if we added a method to Buffer that did the calculation and could be documented better:
Something like
impl Buffer {
..
/// Returns the offset, in bytes, of `Self::ptr` to `Self::data`
///
/// self.ptr and self.data can be different in the following cases:
/// TODO
fn ptr_offset(&self) -> usize {
...
}I was non obvious to me when reading the Buffer code that it was possible for ptr and data to be different
arrow-buffer/src/buffer/immutable.rs
Outdated
| } | ||
|
|
||
| /// Returns the internal byte buffer. | ||
| pub fn get_bytes(&self) -> Arc<Bytes> { |
There was a problem hiding this comment.
Rather than exposing an internal detail, perhaps we can move the offset calculation into Buffer (see suggestion below). In addition to keeping the code more encapsulated, I think it would also likely be faster as it would avoid the Arc clone.
|
Thanks for review @alamb. This is a blocking issue for our first Comet release. Do you think it would be possible to create a patch release of arrow-rs to include this fix (once we are through the review process)? |
| } | ||
| } | ||
|
|
||
| /// Returns the offset, in bytes, of `Self::ptr` to `Self::data` |
| // Handle buffer offset for offset buffer. | ||
| let offset_offset = match data.data_type() { | ||
| DataType::Utf8 | DataType::Binary => { | ||
| // Offset buffer is possible a slice of the buffer. |
There was a problem hiding this comment.
I think this looks much nicer now
arrow-buffer/src/buffer/immutable.rs
Outdated
| /// This function is unsafe as it uses unsafe function `offset_from`. Under certain | ||
| /// conditions, this function can cause undefined behavior. See the documentation of | ||
| /// `offset_from` for more information. | ||
| pub unsafe fn ptr_offset(&self) -> usize { |
There was a problem hiding this comment.
I'm wondering if this function should be unsafe. The ptr should always be in bounds of data, any safe function that modifies ptr should uphold that invariant. If it is not in bounds then nearly all other methods of Buffer would also expose UB.
There was a problem hiding this comment.
Makes sense. slice, advance and slice_with_length even as_ptr are all safe functions.
viirya
force-pushed
the
fix_string_offset_ffi
branch
2 times, most recently
from
4959548 to
c1dd831
Compare
viirya
force-pushed
the
fix_string_offset_ffi
branch
from
c1dd831 to
826e77f
Compare
|
Thanks @alamb @andygrove @jhorstmann |
|
I intend to take a closer look at this tomorrow, as I'm not entirely sure it is correct |
|
Let me know if you have any question about the issue we encountered. |
5 participants
Which issue does this PR close?
Closes #5896.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?