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 3e89669636 fix(arrow-array): align FFI buffers before validation under
force_validate (#10798)
3e89669636 is described below
commit 3e896696360d1b87f0b3a68efc9bf4d427461bd0
Author: Aditya Mishra <[email protected]>
AuthorDate: Fri Sep 4 04:12:41 2026 +0530
fix(arrow-array): align FFI buffers before validation under force_validate
(#10798)
# Which issue does this PR close?
- Closes #10034.
# Rationale for this change
`from_ffi` realigned under-aligned C Data Interface buffers (e.g. an
8-byte aligned `Decimal128` from a JVM producer) *after* `consume()`.
under `force_validate`, `consume()`'s `build()` validates first and
rejects the buffer before the realign runs, so spec-legal input errors.
reachable via the `arrow` crate with `features = ["force_validate",
"ffi"]` calling `arrow::ffi::from_ffi`.
# What changes are included in this PR?
- `ImportedArrowArray::consume` builds through `ArrayDataBuilder` with
`align_buffers(true)` before validation, matching `arrow-ipc`'s
`create_array_from_builder`
- dropped the now-redundant outer `align_buffers()` calls in `from_ffi`
/ `from_ffi_and_data_type`.
# Are these changes tested?
covered by `test_decimal128_under_aligned_round_trip`. the issue
suggested ungating it under `force_validate`, but that isn't possible as
its fixture is a misaligned `ArrayData` built with `build_unchecked`,
which validates under `force_validate` and so rejects the input at
construction, before `from_ffi` runs. the gate stays with a comment
explaining why
# Are there any user-facing changes?
no public API change. behavior only changes under `force_validate`,
where spec-legal under-aligned input is realigned instead of erroring.
---
arrow-array/Cargo.toml | 2 +-
arrow-array/src/array/fixed_size_binary_array.rs | 3 +
arrow-array/src/ffi.rs | 78 +++++++++---------------
3 files changed, 34 insertions(+), 49 deletions(-)
diff --git a/arrow-array/Cargo.toml b/arrow-array/Cargo.toml
index f5d26b5356..b7c7cfabc6 100644
--- a/arrow-array/Cargo.toml
+++ b/arrow-array/Cargo.toml
@@ -69,7 +69,7 @@ all-features = true
[features]
async = ["dep:futures"]
ffi = ["arrow-schema/ffi", "arrow-data/ffi", "dep:libc"]
-force_validate = []
+force_validate = ["arrow-data/force_validate"]
# Enable memory tracking support
pool = ["arrow-buffer/pool", "arrow-data/pool"]
diff --git a/arrow-array/src/array/fixed_size_binary_array.rs
b/arrow-array/src/array/fixed_size_binary_array.rs
index c865c2b342..39bb0f52ff 100644
--- a/arrow-array/src/array/fixed_size_binary_array.rs
+++ b/arrow-array/src/array/fixed_size_binary_array.rs
@@ -1029,6 +1029,9 @@ mod tests {
}
#[test]
+ // Under force_validate `build_unchecked` panics on the invalid child data
+ // before we reach the `FixedSizeBinaryArray::from` path we want to test.
+ #[cfg(not(feature = "force_validate"))]
#[should_panic(expected = "The child array cannot contain null values.")]
fn
test_fixed_size_binary_array_from_fixed_size_list_array_with_child_nulls_failed()
{
let values = [0_u8, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11];
diff --git a/arrow-array/src/ffi.rs b/arrow-array/src/ffi.rs
index 07707a641a..1ee6ee1a9c 100644
--- a/arrow-array/src/ffi.rs
+++ b/arrow-array/src/ffi.rs
@@ -253,14 +253,8 @@ pub unsafe fn from_ffi(array: FFI_ArrowArray, schema:
&FFI_ArrowSchema) -> Resul
data_type: dt,
owner: &array,
};
- let mut data = tmp.consume()?;
- // arrow-rs has stricter alignment requirements than the C Data Interface
spec;
- // a no-op when buffers are already aligned. Unreachable under
- // `cfg(feature = "force_validate")`; tracked in #10034.
- // See https://github.com/apache/arrow/issues/43552 and
- // https://github.com/apache/arrow-rs/issues/10028 for context.
- data.align_buffers();
- Ok(data)
+ // `consume` aligns under-aligned buffers before validating them.
+ tmp.consume()
}
/// Import [ArrayData] from the C Data Interface
@@ -278,14 +272,8 @@ pub unsafe fn from_ffi_and_data_type(
data_type,
owner: &array,
};
- let mut data = tmp.consume()?;
- // arrow-rs has stricter alignment requirements than the C Data Interface
spec;
- // a no-op when buffers are already aligned. Unreachable under
- // `cfg(feature = "force_validate")`; tracked in #10034.
- // See https://github.com/apache/arrow/issues/43552 and
- // https://github.com/apache/arrow-rs/issues/10028 for context.
- data.align_buffers();
- Ok(data)
+ // `consume` aligns under-aligned buffers before validating them.
+ tmp.consume()
}
#[derive(Debug)]
@@ -322,20 +310,22 @@ impl ImportedArrowArray<'_> {
child_data.push(d.consume()?);
}
- // Safety: all fields (length, null_count, null buffer, data buffers,
child data) were
- // derived from the C Data Interface schema and array, which the
caller of `from_ffi`
- // guarantees follow the spec; the constructed `ArrayData` satisfies
its invariants.
- Ok(unsafe {
- ArrayData::new_unchecked(
- self.data_type,
- len,
- null_count,
- null_bit_buffer,
- offset,
- buffers,
- child_data,
- )
- })
+ // Align before validate: spec-legal 8-byte-aligned buffers (e.g.
Decimal128
+ // from JVM) get realigned rather than rejected, even under
`force_validate`.
+ // Mirrors the IPC reader. See #10034.
+ let mut builder = ArrayData::builder(self.data_type)
+ .len(len)
+ .offset(offset)
+ .null_bit_buffer(null_bit_buffer)
+ .buffers(buffers)
+ .child_data(child_data)
+ .align_buffers(true);
+ // Only set the count if the producer reported one; else `build`
recomputes.
+ if let Some(null_count) = null_count {
+ builder = builder.null_count(null_count);
+ }
+ // SAFETY: the caller guarantees the data agrees with the C Data
Interface.
+ unsafe { builder.skip_validation(true) }.build()
}
fn consume_children(&self) -> Result<Vec<ArrayData>> {
@@ -672,33 +662,25 @@ mod tests_to_then_from_ffi {
// case with nulls is tested in the docs, through the example on this
module.
#[test]
- #[cfg(not(feature = "force_validate"))]
- fn test_decimal128_under_aligned_round_trip() -> Result<()> {
- // Construct an 8-aligned-but-not-16-aligned i128 data buffer to model
- // an FFI producer that only guarantees the C Data Interface's
- // recommended 8-byte alignment (e.g. arrow-java).
+ fn test_decimal128_under_aligned_import() -> Result<()> {
+ // FixedSizeBinary(16) needs only 1-byte alignment so it builds cleanly
+ // even under force_validate; imported as Decimal128 it triggers the
+ // realignment path. Regression test for #10034.
let aligned = Buffer::from_vec(vec![0_i128, 1_i128, 2_i128]);
let under_aligned = aligned.slice(8);
assert_eq!(under_aligned.as_ptr().align_offset(8), 0);
assert_ne!(under_aligned.as_ptr().align_offset(16), 0);
- // SAFETY: buffer is large enough for 2 i128 elements; misaligned
- // input is the condition under test.
- let data = unsafe {
- ArrayData::builder(DataType::Decimal128(10, 2))
- .len(2)
- .add_buffer(under_aligned)
- .build_unchecked()
- };
+ let data = ArrayData::builder(DataType::FixedSizeBinary(16))
+ .len(2)
+ .add_buffer(under_aligned)
+ .build()?;
- let schema = FFI_ArrowSchema::try_from(data.data_type()).unwrap();
let array = FFI_ArrowArray::new(&data);
-
- let imported = unsafe { from_ffi(array, &schema) }?;
+ let imported = unsafe { from_ffi_and_data_type(array,
DataType::Decimal128(10, 2)) }?;
let array = Decimal128Array::from(imported);
- // The little-endian byte layout of [0i128, 1, 2] sliced 8 bytes in
- // yields elements `1 << 64` and `2 << 64`.
+ // slicing at byte 8 into [0i128, 1, 2] yields elements 1<<64 and
2<<64.
assert_eq!(array.len(), 2);
assert_eq!(array.value(0), 1_i128 << 64);
assert_eq!(array.value(1), 2_i128 << 64);
