Remove undefined behavior in value method of boolean and primitive …

…arrays (#644) (#668)

* Remove UB in `value`

* Add safety note

Co-authored-by: Daniël Heres <[email protected]>

arrow/src/array/array_boolean.rs

@@ -115,9 +115,11 @@ impl BooleanArray {
 
     /// Returns the boolean value at index `i`.
     ///
-    /// Note this doesn't do any bound checking, for performance reason.
+    /// Panics of offset `i` is out of bounds
     pub fn value(&self, i: usize) -> bool {
-        debug_assert!(i < self.len());
+        assert!(i < self.len());
+        // Safety:
+        // `i < self.len()
         unsafe { self.value_unchecked(i) }
     }
 }

arrow/src/array/array_primitive.rs

@@ -101,12 +101,10 @@ impl<T: ArrowPrimitiveType> PrimitiveArray<T> {
 
     /// Returns the primitive value at index `i`.
     ///
-    /// Note this doesn't do any bound checking, for performance reason.
-    /// # Safety
-    /// caller must ensure that the passed in offset is less than the array len()
+    /// Panics of offset `i` is out of bounds
     #[inline]
     pub fn value(&self, i: usize) -> T::Native {
-        debug_assert!(i < self.len());
+        assert!(i < self.len());
         unsafe { self.value_unchecked(i) }
     }
 

arrow/src/array/array_string.rs

@@ -81,6 +81,7 @@ impl<OffsetSize: StringOffsetSizeTrait> GenericStringArray<OffsetSize> {
     /// Returns the element at index
     /// # Safety
     /// caller is responsible for ensuring that index is within the array bounds
+    #[inline]
     pub unsafe fn value_unchecked(&self, i: usize) -> &str {
         let end = self.value_offsets().get_unchecked(i + 1);
         let start = self.value_offsets().get_unchecked(i);
@@ -103,28 +104,12 @@ impl<OffsetSize: StringOffsetSizeTrait> GenericStringArray<OffsetSize> {
     }
 
     /// Returns the element at index `i` as &str
+    #[inline]
     pub fn value(&self, i: usize) -> &str {
         assert!(i < self.data.len(), "StringArray out of bounds access");
-        //Soundness: length checked above, offset buffer length is 1 larger than logical array length
-        let end = unsafe { self.value_offsets().get_unchecked(i + 1) };
-        let start = unsafe { self.value_offsets().get_unchecked(i) };
-
-        // Soundness
-        // pointer alignment & location is ensured by RawPtrBox
-        // buffer bounds/offset is ensured by the value_offset invariants
-        // ISSUE: utf-8 well formedness is not checked
-        unsafe {
-            // 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
-            let slice = std::slice::from_raw_parts(
-                self.value_data.as_ptr().offset(start.to_isize().unwrap()),
-                (*end - *start).to_usize().unwrap(),
-            );
-            std::str::from_utf8_unchecked(slice)
-        }
+        // Safety:
+        // `i < self.data.len()
+        unsafe { self.value_unchecked(i) }
     }
 
     fn from_list(v: GenericListArray<OffsetSize>) -> Self {

arrow/src/compute/kernels/comparison.rs

@@ -46,7 +46,10 @@ macro_rules! compare_op {
         let null_bit_buffer =
             combine_option_bitmap($left.data_ref(), $right.data_ref(), $left.len())?;
 
-        let comparison = (0..$left.len()).map(|i| $op($left.value(i), $right.value(i)));
+        // Safety:
+        // `i < $left.len()` and $left.len() == $right.len()
+        let comparison = (0..$left.len())
+            .map(|i| unsafe { $op($left.value_unchecked(i), $right.value_unchecked(i)) });
         // same size as $left.len() and $right.len()
         let buffer = unsafe { MutableBuffer::from_trusted_len_iter_bool(comparison) };
 
@@ -121,8 +124,10 @@ macro_rules! compare_op_primitive {
 macro_rules! compare_op_scalar {
     ($left: expr, $right:expr, $op:expr) => {{
         let null_bit_buffer = $left.data().null_buffer().cloned();
-
-        let comparison = (0..$left.len()).map(|i| $op($left.value(i), $right));
+        // Safety:
+        // `i < $left.len()`
+        let comparison =
+            (0..$left.len()).map(|i| unsafe { $op($left.value_unchecked(i), $right) });
         // same as $left.len()
         let buffer = unsafe { MutableBuffer::from_trusted_len_iter_bool(comparison) };