[GitHub] [arrow] tyrelr commented on a change in pull request #10046: ARROW-12398: [Rust] remove redundant bound check in iterators

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

tyrelr commented on a change in pull request #10046:
URL: https://github.com/apache/arrow/pull/10046#discussion_r615255644



##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -56,7 +56,7 @@ impl<'a, T: ArrowPrimitiveType> std::iter::Iterator for PrimitiveIter<'a, T> {
         } else {
             let old = self.current;
             self.current += 1;
-            Some(Some(self.array.value(old)))
+            unsafe { Some(Some(self.array.value_unchecked(old))) }

Review comment:
       Missing safety comment

##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -118,7 +118,9 @@ impl<'a> std::iter::Iterator for BooleanIter<'a> {
         } else {
             let old = self.current;
             self.current += 1;
-            Some(Some(self.array.value(old)))
+            // Safety:
+            // we just checked bounds

Review comment:
       I've been wanting to change this, thanks for beating me to it :)
   
   It's not critical, but could this clarify that the bounds check happens in the self.current_end comparison?  It seems to me this also depends on an undocumented struct invariant that self.current_end <= array.len().  Upon reading the comment, I had assumed that is_null was expected to be the bounds check, but ArrayData.is_null short circuits to false if there is no null buffer, so wouldn't always bounds check.  (this also applies to the other safety comments)

##########
File path: rust/arrow/src/array/iterator.rs
##########
@@ -77,7 +77,7 @@ impl<'a, T: ArrowPrimitiveType> std::iter::DoubleEndedIterator for PrimitiveIter
             Some(if self.array.is_null(self.current_end) {
                 None
             } else {
-                Some(self.array.value(self.current_end))
+                unsafe { Some(self.array.value_unchecked(self.current_end)) }

Review comment:
       Missing safety comment




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

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