Conversation
…::MAX value_offset narrows a usize offset to i32 with as, so past i32::MAX child values it returns a negative number. The avro FixedSizeList encoder cast that back to usize and panicked writing the batch. Deprecate it in favour of i * value_length() as usize, as was done for FixedSizeBinaryArray::value_offset, and panic instead of wrapping. The avro encoder and take work the offset out in usize, and take returns an error when it does not fit its u32 indices. Closes apache#11059.
github-actions
Bot
added
arrow
| /// # Panics | ||
| /// | ||
| /// Panics if the offset exceeds `i32::MAX`. | ||
| #[deprecated(since = "60.1.0", note = "Use i * value_length() as usize instead")] |
There was a problem hiding this comment.
| #[deprecated(since = "60.1.0", note = "Use i * value_length() as usize instead")] | |
| #[deprecated(since = "60.0.0", note = "Use i * value_length() as usize instead")] |
There was a problem hiding this comment.
we could also make value_offset_at public and point to that, but considering its just a thin wrapper over the multiplication and we don't use value_offset much in the codebase, its probably fine to leave it private to not expand our API surface 👍
There was a problem hiding this 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
There was a problem hiding this 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.
| /// # Panics | ||
| /// | ||
| /// Panics if the offset exceeds `i32::MAX`. | ||
| #[deprecated(since = "60.1.0", note = "Use i * value_length() as usize instead")] |
There was a problem hiding this 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
| #[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") |
There was a problem hiding this 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 🤔 )
There was a problem hiding this 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.
There was a problem hiding this comment.
for a minor release I think we should just deprecate the function and not change its behavior
Jefffrey asked for 60.0.0 rather than 60.1.0, and alamb asked for a public function that works the offset out correctly, so value_offset_at is public now and the deprecation note points at it.
3 participants
Which issue does this PR close?
Rationale for this change
FixedSizeListArray::value_offset casts a usize offset to i32, so once a row starts past i32::MAX child values it returns a negative number. the avro FixedSizeList encoder cast that back to usize and panicked writing the batch.
What changes are included in this PR?
Are these changes tested?
yes. fixed_size_list_encoder_int32_sliced is new and covers the avro change. the overflow itself needs 384 MiB so it isn't a unit test, but the repro from the issue panics on 59.3.0 and writes the batch on this branch. arrow-array 972, arrow-select 446 and arrow-avro 515 tests pass, and clippy is clean.
Are there any user-facing changes?
FixedSizeListArray::value_offset is deprecated and panics past i32::MAX. take on a FixedSizeList returns an error past u32::MAX instead of wrapping.