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:
[email protected]
