alamb commented on code in PR #11070:
URL: https://github.com/apache/arrow-rs/pull/11070#discussion_r4019876664
##########
arrow-array/src/array/fixed_size_list_array.rs:
##########
@@ -371,9 +371,14 @@ impl FixedSizeListArray {
/// Returns the offset for value at index `i`.
///
/// Note this doesn't do any bound checking, for performance reason.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the offset exceeds `i32::MAX`.
+ #[deprecated(since = "60.1.0", note = "Use i * value_length() as usize
instead")]
#[inline]
pub fn value_offset(&self, i: usize) -> i32 {
- self.value_offset_at(i) as i32
+ i32::try_from(self.value_offset_at(i)).expect("offset overflow")
Review Comment:
It would be somewhat annoying if I were a user to start seeing panics (even
though you could argue that is better than silent overflows 🤔 )
##########
arrow-array/src/array/fixed_size_list_array.rs:
##########
@@ -371,9 +371,14 @@ impl FixedSizeListArray {
/// Returns the offset for value at index `i`.
///
/// Note this doesn't do any bound checking, for performance reason.
+ ///
+ /// # Panics
+ ///
+ /// Panics if the offset exceeds `i32::MAX`.
+ #[deprecated(since = "60.1.0", note = "Use i * value_length() as usize
instead")]
Review Comment:
I do think the wrapping / offset calculations is somewhat tricky -- so
exposing a function that does it the right way seems good 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
