Cintu07 commented on code in PR #11070:
URL: https://github.com/apache/arrow-rs/pull/11070#discussion_r4021998201
##########
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:
taken, and value_offset_at is public now with the note pointing at it. the
multiplication is thin but the i32 narrowing is exactly what this issue was, so
a public usize accessor is the thing to point people at rather than asking
every caller to rewrite it correctly themselves.
##########
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:
fair. the deprecated method only panics past i32::MAX, where it used to hand
back a negative offset and the avro writer turned that into an index near
u64::MAX. FixedSizeBinaryArray::value_offset documents the same panic since
59.0.0. anyone who wants no panic at all can move to value_offset_at, which
stays in usize. happy to leave the wrapping cast alone and only deprecate if
you would rather not add a panic at all.
--
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]
