tyrelr commented on a change in pull request #9215:
URL: https://github.com/apache/arrow/pull/9215#discussion_r563166348
##########
File path: rust/arrow/src/array/array_string.rs
##########
@@ -50,49 +50,65 @@ pub struct GenericStringArray<OffsetSize:
StringOffsetSizeTrait> {
}
impl<OffsetSize: StringOffsetSizeTrait> GenericStringArray<OffsetSize> {
- /// Returns the offset for the element at index `i`.
- ///
- /// Note this doesn't do any bound checking, for performance reason.
- #[inline]
- pub fn value_offset(&self, i: usize) -> OffsetSize {
- self.value_offset_at(self.data.offset() + i)
- }
-
/// Returns the length for the element at index `i`.
- ///
- /// Note this doesn't do any bound checking, for performance reason.
#[inline]
- pub fn value_length(&self, mut i: usize) -> OffsetSize {
- i += self.data.offset();
- self.value_offset_at(i + 1) - self.value_offset_at(i)
+ pub fn value_length(&self, i: usize) -> OffsetSize {
+ let offsets = self.value_offsets();
+ offsets[i + 1] - offsets[i]
}
- /// Returns a clone of the value offset buffer
- pub fn value_offsets(&self) -> Buffer {
- self.data.buffers()[0].clone()
+ /// Returns the offset values in the offsets buffer
+ #[inline]
+ pub fn value_offsets(&self) -> &[OffsetSize] {
Review comment:
I had considered that, but was concerned that it may be confused with
the buffer.offset... my hope was that it'd make it clear that it's an "offset
into values()" (not into the buffer, which has the additional array-slicing
offset).
I could go either way with it though?
----------------------------------------------------------------
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]
