This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new 132371adb7 fix(arrow-array): deprecate
FixedSizeListArray::value_offset, which wraps past i32::MAX (#11070)
132371adb7 is described below
commit 132371adb7c3b02abfe188e466087a38e6d005dc
Author: pawan <[email protected]>
AuthorDate: Wed Sep 23 07:59:01 2026 +0530
fix(arrow-array): deprecate FixedSizeListArray::value_offset, which wraps
past i32::MAX (#11070)
### Which issue does this PR close?
- Closes #11059.
### 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?
- deprecate value_offset in favour of i * value_length() as usize, same
as FixedSizeBinaryArray::value_offset in #9910, and panic instead of
wrapping
- the avro encoder uses idx * elem_len instead
- take_value_indices_from_fixed_size_list computes the offset in usize
and returns an error when it doesn't fit u32
- the value_offset asserts come out of the tests, and the slice test
checks row values instead
### 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.
---
arrow-array/src/array/fixed_size_list_array.rs | 21 ++++++++++----
arrow-array/src/builder/fixed_size_list_builder.rs | 12 ++++----
arrow-avro/src/writer/encoder.rs | 32 ++++++++++++++++++++--
arrow-select/src/take.rs | 4 ++-
4 files changed, 53 insertions(+), 16 deletions(-)
diff --git a/arrow-array/src/array/fixed_size_list_array.rs
b/arrow-array/src/array/fixed_size_list_array.rs
index de7b97c231..68c4196424 100644
--- a/arrow-array/src/array/fixed_size_list_array.rs
+++ b/arrow-array/src/array/fixed_size_list_array.rs
@@ -371,6 +371,11 @@ impl FixedSizeListArray {
/// Returns the offset for value at index `i`.
///
/// Note this doesn't do any bound checking, for performance reason.
+ ///
+ /// Past `i32::MAX` child elements the offset does not fit the return type
+ /// and wraps negative. Use [`Self::value_offset_at`], which stays in
+ /// `usize`.
+ #[deprecated(since = "60.0.0", note = "Use value_offset_at instead")]
#[inline]
pub fn value_offset(&self, i: usize) -> i32 {
self.value_offset_at(i) as i32
@@ -384,8 +389,12 @@ impl FixedSizeListArray {
self.value_length
}
+ /// Returns the offset in the child array of the value at index `i`, in
+ /// elements.
+ ///
+ /// Note this doesn't do any bound checking, for performance reason.
#[inline]
- const fn value_offset_at(&self, i: usize) -> usize {
+ pub const fn value_offset_at(&self, i: usize) -> usize {
i * self.value_length as usize
}
@@ -662,7 +671,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(3, list_array.len());
assert_eq!(0, list_array.null_count());
- assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(6, list_array.value_offset_at(2));
assert_eq!(3, list_array.value_length());
assert_eq!(0,
list_array.value(0).as_primitive::<Int32Type>().value(0));
for i in 0..3 {
@@ -684,7 +693,7 @@ mod tests {
assert_eq!(2, list_array.len());
assert_eq!(0, list_array.null_count());
assert_eq!(3,
list_array.value(0).as_primitive::<Int32Type>().value(0));
- assert_eq!(3, list_array.value_offset(1));
+ assert_eq!(3, list_array.value_offset_at(1));
assert_eq!(3, list_array.value_length());
}
@@ -745,7 +754,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(5, list_array.len());
assert_eq!(2, list_array.null_count());
- assert_eq!(6, list_array.value_offset(3));
+ assert_eq!(6, list_array.value_offset_at(3));
assert_eq!(2, list_array.value_length());
let sliced_array = list_array.slice(1, 4);
@@ -766,8 +775,8 @@ mod tests {
.downcast_ref::<FixedSizeListArray>()
.unwrap();
assert_eq!(2, sliced_list_array.value_length());
- assert_eq!(4, sliced_list_array.value_offset(2));
- assert_eq!(6, sliced_list_array.value_offset(3));
+ assert_eq!(4, sliced_list_array.value_offset_at(2));
+ assert_eq!(6, sliced_list_array.value_offset_at(3));
}
#[test]
diff --git a/arrow-array/src/builder/fixed_size_list_builder.rs
b/arrow-array/src/builder/fixed_size_list_builder.rs
index a297a5423d..f3cec5c43f 100644
--- a/arrow-array/src/builder/fixed_size_list_builder.rs
+++ b/arrow-array/src/builder/fixed_size_list_builder.rs
@@ -310,7 +310,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(4, list_array.len());
assert_eq!(1, list_array.null_count());
- assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(6, list_array.value_offset_at(2));
assert_eq!(3, list_array.value_length());
}
@@ -323,7 +323,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(4, list_array.len());
assert_eq!(0, list_array.null_count());
- assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(6, list_array.value_offset_at(2));
assert_eq!(3, list_array.value_length());
}
@@ -336,7 +336,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(4, list_array.len());
assert_eq!(1, list_array.null_count());
- assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(6, list_array.value_offset_at(2));
assert_eq!(3, list_array.value_length());
}
@@ -383,7 +383,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(4, list_array.len());
assert_eq!(1, list_array.null_count());
- assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(6, list_array.value_offset_at(2));
assert_eq!(3, list_array.value_length());
}
@@ -405,7 +405,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(4, list_array.len());
assert_eq!(1, list_array.null_count());
- assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(6, list_array.value_offset_at(2));
assert_eq!(3, list_array.value_length());
}
@@ -442,7 +442,7 @@ mod tests {
assert_eq!(DataType::Int32, list_array.value_type());
assert_eq!(6, list_array.len());
assert_eq!(2, list_array.null_count());
- assert_eq!(6, list_array.value_offset(2));
+ assert_eq!(6, list_array.value_offset_at(2));
assert_eq!(3, list_array.value_length());
}
diff --git a/arrow-avro/src/writer/encoder.rs b/arrow-avro/src/writer/encoder.rs
index 2cbd913aca..7ce57c964f 100644
--- a/arrow-avro/src/writer/encoder.rs
+++ b/arrow-avro/src/writer/encoder.rs
@@ -2100,7 +2100,6 @@ impl<'a, O: OffsetSizeTrait> ListViewEncoder<'a, O> {
/// FixedSizeList encoder.
struct FixedSizeListEncoder<'a> {
- list: &'a FixedSizeListArray,
values: FieldEncoder<'a>,
values_offset: usize,
elem_len: usize,
@@ -2113,7 +2112,6 @@ impl<'a> FixedSizeListEncoder<'a> {
item_plan: &FieldPlan,
) -> Result<Self, AvroError> {
Ok(Self {
- list,
values: FieldEncoder::make_encoder(
list.values().as_ref(),
item_plan,
@@ -2126,7 +2124,7 @@ impl<'a> FixedSizeListEncoder<'a> {
fn encode<W: Write + ?Sized>(&mut self, out: &mut W, idx: usize) ->
Result<(), AvroError> {
// Starting index is relative to values() start
- let rel = self.list.value_offset(idx) as usize;
+ let rel = idx * self.elem_len;
let start = self.values_offset + rel;
let end = start + self.elem_len;
encode_blocked_range(out, start, end, |out, row| {
@@ -2560,6 +2558,34 @@ mod tests {
assert_bytes_eq(&got, &expected);
}
+ #[test]
+ fn fixed_size_list_encoder_int32_sliced() {
+ // [[1, 2], [3, 4], [5, 6]] sliced to [[3, 4], [5, 6]]
+ let values = Int32Array::from(vec![1, 2, 3, 4, 5, 6]);
+ let list = FixedSizeListArray::new(
+ Field::new("item", DataType::Int32, true).into(),
+ 2,
+ Arc::new(values) as ArrayRef,
+ None,
+ )
+ .slice(1, 2);
+ let mut expected = Vec::new();
+ for row in [[3, 4], [5, 6]] {
+ expected.extend(avro_long_bytes(2));
+ for value in row {
+ expected.extend(avro_long_bytes(value));
+ }
+ expected.extend(avro_long_bytes(0));
+ }
+
+ let plan = FieldPlan::List {
+ items_nullability: None,
+ item_plan: Box::new(FieldPlan::Scalar),
+ };
+ let got = encode_all(&list, &plan, None);
+ assert_bytes_eq(&got, &expected);
+ }
+
#[test]
fn struct_encoder_two_fields() {
// Struct { a: Int32, b: Utf8 }
diff --git a/arrow-select/src/take.rs b/arrow-select/src/take.rs
index 5cbf8bcd87..f52774825c 100644
--- a/arrow-select/src/take.rs
+++ b/arrow-select/src/take.rs
@@ -1467,7 +1467,9 @@ where
.value(i)
.to_usize()
.ok_or_else(|| ArrowError::ComputeError("Cast to usize
failed".to_string()))?;
- let start = list.value_offset(index) as <UInt32Type as
ArrowPrimitiveType>::Native;
+ let start = u32::try_from(list.value_offset_at(index)).map_err(|_|
{
+ ArrowError::ComputeError("FixedSizeList offset overflows
u32".to_string())
+ })?;
// Safety: Range always has known length.
unsafe {
