adamreeve commented on code in PR #9850:
URL: https://github.com/apache/arrow-rs/pull/9850#discussion_r3164949680
##########
arrow-array/src/array/fixed_size_binary_array.rs:
##########
@@ -186,7 +185,7 @@ impl FixedSizeBinaryArray {
/// # Safety
///
/// Caller is responsible for ensuring that the index is within the bounds
- /// of the array
+ /// of the array and the resulting byte offset fits in `i32`
pub unsafe fn value_unchecked(&self, i: usize) -> &[u8] {
Review Comment:
Would it make sense to add new methods that are alternatives to
`value_offset` and `value_offset_at` that return `usize`, so we don't need this
limitation? Or at least update this method so it doesn't use them and doesn't
suffer from i32 overflow. Because with this change, `value` now handles when
`value_offset` is greater than max i32, but `value_unchecked` still doesn't.
I would expect `value_unchecked` to work correctly for all cases where
`value` doesn't panic.
And existing code that uses `value_unchecked` might validate the index but
not be aware of this hidden safety requirement.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
