You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2024/08/25 13:36:24 UTC
[PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
alamb opened a new pull request, #6306:
URL: https://github.com/apache/arrow-rs/pull/6306
# Which issue does this PR close?
Follow on to https://github.com/apache/arrow-rs/pull/6231 from @xinlifoobar
# Rationale for this change
These new APIs were added in https://github.com/apache/arrow-rs/pull/6231 and I wanted to clarify their behavior (so we can potentailly use them in DataFusion)
# What changes are included in this PR?
Update doc comments
# Are there any user-facing changes?
Documentation, no functional 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#issuecomment-2315026531
Ok, merging in these docs so they match the current code. As @tustvold says if the code needs to be updated we can also update the docs
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#discussion_r1731703196
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -317,8 +317,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
})
}
- /// Returns an iterator over the prefix bytes of this array with respect to the prefix length.
- /// If the prefix length is larger than the string length, it will return the empty slice.
+ /// Returns an iterator over the first `prefix_len` bytes of each array
+ /// element, including null values.
+ ///
+ /// If `prefix_len` is larger than the element's length, the iterator will
Review Comment:
I guess I would have thought returning `Option<&[u8]>` would be perhaps more intuitive and likely perform similarly? That being said I am surprised that skipping the null mask yields performance returns, as this hasn't been my experience with regular StringArray, so my intuition about performance may be off
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "xinlifoobar (via GitHub)" <gi...@apache.org>.
xinlifoobar commented on code in PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#discussion_r1732795874
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -317,8 +317,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
})
}
- /// Returns an iterator over the prefix bytes of this array with respect to the prefix length.
- /// If the prefix length is larger than the string length, it will return the empty slice.
+ /// Returns an iterator over the first `prefix_len` bytes of each array
+ /// element, including null values.
+ ///
+ /// If `prefix_len` is larger than the element's length, the iterator will
Review Comment:
Thanks for the suggestions! @tustvold.
I created a new PR #6312 to put this discussion. I did this in the early version but didnot see performance gains like this time - it did well for inline values.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#discussion_r1732836274
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -317,8 +317,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
})
}
- /// Returns an iterator over the prefix bytes of this array with respect to the prefix length.
- /// If the prefix length is larger than the string length, it will return the empty slice.
+ /// Returns an iterator over the first `prefix_len` bytes of each array
+ /// element, including null values.
+ ///
+ /// If `prefix_len` is larger than the element's length, the iterator will
Review Comment:
I left some comments, I think this API would be better if it returned None for a prefix too long, even if it continues to not inspect null masks
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#discussion_r1730950065
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -317,8 +317,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
})
}
- /// Returns an iterator over the prefix bytes of this array with respect to the prefix length.
- /// If the prefix length is larger than the string length, it will return the empty slice.
+ /// Returns an iterator over the first `prefix_len` bytes of each array
+ /// element, including null values.
+ ///
+ /// If `prefix_len` is larger than the element's length, the iterator will
Review Comment:
This feels rather surprising to me?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#discussion_r1732827235
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -317,8 +317,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
})
}
- /// Returns an iterator over the prefix bytes of this array with respect to the prefix length.
- /// If the prefix length is larger than the string length, it will return the empty slice.
+ /// Returns an iterator over the first `prefix_len` bytes of each array
+ /// element, including null values.
+ ///
+ /// If `prefix_len` is larger than the element's length, the iterator will
Review Comment:
The results on https://github.com/apache/arrow-rs/pull/6312 seem to show that using null masks does indeed reduce performance
So is the conclusion that these APIs are good to go?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "xinlifoobar (via GitHub)" <gi...@apache.org>.
xinlifoobar commented on code in PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#discussion_r1732795874
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -317,8 +317,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
})
}
- /// Returns an iterator over the prefix bytes of this array with respect to the prefix length.
- /// If the prefix length is larger than the string length, it will return the empty slice.
+ /// Returns an iterator over the first `prefix_len` bytes of each array
+ /// element, including null values.
+ ///
+ /// If `prefix_len` is larger than the element's length, the iterator will
Review Comment:
Thanks for the suggestions! @tustvold.
I created a new PR #6312 to put this discussion. I did this in the early version but didn't see performance gains like this time - it did well for inline values. We might do a combination based on the bench result, e.g., null masks for prefix_iter and slice for suffix_iter.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Minor: Improve comments on GenericByteViewArray::bytes_iter(), prefix_iter() and suffix_iter() [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6306:
URL: https://github.com/apache/arrow-rs/pull/6306#discussion_r1731691116
##########
arrow-array/src/array/byte_view_array.rs:
##########
@@ -317,8 +317,11 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
})
}
- /// Returns an iterator over the prefix bytes of this array with respect to the prefix length.
- /// If the prefix length is larger than the string length, it will return the empty slice.
+ /// Returns an iterator over the first `prefix_len` bytes of each array
+ /// element, including null values.
+ ///
+ /// If `prefix_len` is larger than the element's length, the iterator will
Review Comment:
Is that a statement or a question?
If you have a suggestion of how to make it less surprising perhaps @xinlifoobar would be willing to give it a try
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org