jorgecarleitao commented on a change in pull request #9215:
URL: https://github.com/apache/arrow/pull/9215#discussion_r562862070
##########
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 think that I covered all of those in #9291 . Good that there is no
impact on `sort`, which was the only case where I could not optimize (i.e. we
really needed the bound check).
The larger task atm is to convert all unit-tests that use `value(i)` to
`values()[i]`.
Fyi, the reason I propose keeping `value(i)` as `unsafe` instead of
un-performant is that there is no way to efficiently access the value `i`
without knowing the internals of the primitiveArray (i.e. pointer and offset).
imo this method does serve the purpose of abstracting out those details. It
just happens to be `unsafe` and we currently not marking it as such. However,
please say if you disagree here.
wrt to the `comparison kernel`, we can create a generic for primitives alone
for that use-case, no?
Let me know what do you think
----------------------------------------------------------------
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]
