fix: Adjust FFI_ArrowArray offset based on the offset of offset buffer #5895

viirya · 2024-06-15T21:04:42Z

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?

alamb

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

alamb · 2024-06-17T16:37:36Z

arrow-array/src/ffi.rs

+    }
+
+    #[test]
+    fn test_extend_imported_string_slice() {


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

alamb · 2024-06-17T16:38:27Z

arrow-array/src/ffi.rs

@@ -1458,4 +1460,58 @@ mod tests_from_ffi {

        test_round_trip(&imported_array.consume()?)
    }
+
+    fn export_string(array: StringArray) -> StringArray {


Perhaps calling this function "roundtrip_string_array" would better describe what it does

Yes, it looks better.

alamb · 2024-06-17T16:48:59Z

arrow-data/src/ffi.rs

+                // 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


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

alamb · 2024-06-17T16:51:24Z

arrow-buffer/src/buffer/immutable.rs

@@ -71,6 +71,11 @@ impl Buffer {
        }
    }

+    /// Returns the internal byte buffer.
+    pub fn get_bytes(&self) -> Arc<Bytes> {


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.

Good suggestion.

andygrove · 2024-06-17T17:55:11Z

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)?

alamb · 2024-06-17T19:02:10Z

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)?

For sure -- I filed #5906 to track the idea

alamb · 2024-06-17T20:16:53Z

arrow-buffer/src/buffer/immutable.rs

@@ -71,6 +71,24 @@ impl Buffer {
        }
    }

+    /// Returns the offset, in bytes, of `Self::ptr` to `Self::data`


alamb · 2024-06-17T20:17:47Z

arrow-data/src/ffi.rs

+        // 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.


I think this looks much nicer now

jhorstmann · 2024-06-18T09:47:23Z

arrow-buffer/src/buffer/immutable.rs

+    /// 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 {


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.

Makes sense. slice, advance and slice_with_length even as_ptr are all safe functions.

viirya · 2024-06-20T01:43:15Z

Thanks @alamb @andygrove @jhorstmann

tustvold · 2024-06-25T23:46:42Z

I intend to take a closer look at this tomorrow, as I'm not entirely sure it is correct

viirya · 2024-06-26T01:52:53Z

Let me know if you have any question about the issue we encountered.

github-actions bot added the arrow Changes to the arrow crate label Jun 15, 2024

viirya mentioned this pull request Jun 15, 2024

Out of range when extending on a slice of string array imported through FFI #5896

Closed

fix: Adjust FFI_ArrowArray offset based on the offset of offset buffer

eb51190

viirya force-pushed the fix_string_offset_ffi branch from 60c3e6c to eb51190 Compare June 15, 2024 21:36

alamb approved these changes Jun 17, 2024

View reviewed changes

alamb mentioned this pull request Jun 17, 2024

fix(ipc): reencode list offsets bug #5808

Closed

alamb mentioned this pull request Jun 17, 2024

[DISCUSS] Release arrow-rs / parquet patch release 52.0.1 #5906

Closed

1 task

viirya added 2 commits June 17, 2024 12:43

For review

017f6cb

Fix clippy

fe54f67

alamb approved these changes Jun 17, 2024

View reviewed changes

jhorstmann reviewed Jun 18, 2024

View reviewed changes

viirya force-pushed the fix_string_offset_ffi branch 2 times, most recently from 4959548 to c1dd831 Compare June 18, 2024 15:17

For review

826e77f

viirya force-pushed the fix_string_offset_ffi branch from c1dd831 to 826e77f Compare June 18, 2024 15:48

viirya merged commit 20a569a into apache:master Jun 20, 2024
25 checks passed

viirya deleted the fix_string_offset_ffi branch June 20, 2024 01:43

viirya mentioned this pull request Jun 24, 2024

test: Add unit test for extending slice of list array #5948

Merged

viirya mentioned this pull request Jun 26, 2024

feat: Add ffi_enforce_no_offset feature as a workaround hack to unavailable offset support in Arrow Java #5958

Closed

tustvold mentioned this pull request Jun 26, 2024

Fix FFI array offset handling #5964

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #5895

viirya commented Jun 15, 2024 •

edited

Loading

alamb left a comment

alamb Jun 17, 2024

alamb Jun 17, 2024

viirya Jun 17, 2024

alamb Jun 17, 2024

alamb Jun 17, 2024

viirya Jun 17, 2024

andygrove commented Jun 17, 2024

alamb commented Jun 17, 2024

alamb Jun 17, 2024

alamb Jun 17, 2024

jhorstmann Jun 18, 2024

viirya Jun 18, 2024

viirya commented Jun 20, 2024

tustvold commented Jun 25, 2024

viirya commented Jun 26, 2024

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 #5895

Conversation

viirya commented Jun 15, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Jun 17, 2024

alamb commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Jun 20, 2024

tustvold commented Jun 25, 2024

viirya commented Jun 26, 2024

viirya commented Jun 15, 2024 •

edited

Loading