tyrelr commented on a change in pull request #9215:
URL: https://github.com/apache/arrow/pull/9215#discussion_r561918229
##########
File path: rust/arrow/src/array/array_primitive.rs
##########
@@ -86,13 +86,9 @@ 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()
+ #[inline]
pub fn value(&self, i: usize) -> T::Native {
- let offset = i + self.offset();
- unsafe { *self.raw_values.as_ptr().add(offset) }
+ self.values()[i]
Review comment:
I agree, that we want to guide API users to switch to values(). My
hesitancy with making/leaving it unsafe is that I think it's the only Array
value(x) method when combined with the other changes in this PR (which is why
it got added as a followon commit). That leads to awkwardness in macros, such
as the comparison kernel, where it effectively calls "$left.value(x)", it
could be a papercut for macro-style invocation.
(There may be an argument that a trait could abstract away the need for
macros altogether... but my experiments to rework the comparison kernel to that
effect are incomplete - and partly sidelined with various other experiments I'm
toying with)
I would expect that in the exact for loop you mention, the compiler could
recognize the satisfied loop bound since both len() and .value(i) are inlinable
and use the same underlying field, but I expect it is also easy to complicate
on that loop to defeat that by having a less direct relationship between i &
array.len(). The pretty printing logic probably would do additional bounds
checks now, as it iterates based on len() from the record batch, which is
semantically equal, but nothing in the code enforces that in a way I would
expect the compiler to understand.
I'll launch some benchmarks of head, master, and the commit just before the
PrimitiveArray safety change.
----------------------------------------------------------------
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]
