[GitHub] [arrow-rs] tustvold commented on a diff in pull request #1825: Replace RawPtrBox with ScalarBuffer (#1811)

[GitBox <gi...@apache.org>]

tustvold commented on code in PR #1825:
URL: https://github.com/apache/arrow-rs/pull/1825#discussion_r893302923


##########
arrow/src/array/array_binary.rs:
##########
@@ -64,67 +64,40 @@ impl<OffsetSize: OffsetSizeTrait> GenericBinaryArray<OffsetSize> {
 
     /// Returns a clone of the value data buffer
     pub fn value_data(&self) -> Buffer {
-        self.data.buffers()[1].clone()
+        self.value_data.clone()
     }
 
     /// Returns the offset values in the offsets buffer
     #[inline]
     pub fn value_offsets(&self) -> &[OffsetSize] {
-        // Soundness
-        //     pointer alignment & location is ensured by RawPtrBox
-        //     buffer bounds/offset is ensured by the ArrayData instance.
-        unsafe {
-            std::slice::from_raw_parts(
-                self.value_offsets.as_ptr().add(self.data.offset()),
-                self.len() + 1,
-            )
-        }
+        &self.value_offsets
     }
 
     /// Returns the element at index `i` as bytes slice
     /// # Safety
     /// Caller is responsible for ensuring that the index is within the bounds of the array
     pub unsafe fn value_unchecked(&self, i: usize) -> &[u8] {
-        let end = *self.value_offsets().get_unchecked(i + 1);
-        let start = *self.value_offsets().get_unchecked(i);
-
-        // Soundness
-        // pointer alignment & location is ensured by RawPtrBox
-        // buffer bounds/offset is ensured by the value_offset invariants
-
         // Safety of `to_isize().unwrap()`
         // `start` and `end` are &OffsetSize, which is a generic type that implements the
         // OffsetSizeTrait. Currently, only i32 and i64 implement OffsetSizeTrait,
         // both of which should cleanly cast to isize on an architecture that supports
         // 32/64-bit offsets
-        std::slice::from_raw_parts(
-            self.value_data.as_ptr().offset(start.to_isize().unwrap()),
-            (end - start).to_usize().unwrap(),
-        )
+        let start = self.value_offsets.get_unchecked(i).to_isize().unwrap();

Review Comment:
   I don't like this `to_isize()` dance, really I just want `ArrowNativeType` to provide `AsPrimitive` so that we can do an unchecked cast. Will file a follow up PR



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org