Follow-up to #10030 / #10028.
Describe the bug
arrow::ffi::from_ffi and from_ffi_and_data_type call data.align_buffers() after ImportedArrowArray::consume() to realign protocol-legal but under-aligned C Data Interface buffers (e.g. 8-byte-aligned Decimal128 from a JVM producer). Under cfg(feature = "force_validate") this realignment is unreachable: consume() constructs ArrayData via ArrayData::new_unchecked, which goes through ArrayDataBuilder::build. That builder runs validate_data() whenever force_validate is enabled, regardless of skip_validation:
// arrow-data/src/data.rs:2195
if !skip_validation.get() || cfg!(feature = "force_validate") {
data.validate_data()?;
}
validate() rejects misaligned BufferSpec::FixedWidth buffers (arrow-data/src/data.rs:842-867), so consume()? returns InvalidArgumentError before from_ffi's align_buffers() runs. The same applies recursively through consume_children / consume_child.
The new regression test added in #10030 (test_decimal128_under_aligned_round_trip) is gated #[cfg(not(feature = "force_validate"))], so CI under that feature does not surface the gap.
This is not a regression introduced by #10030 — prior to that PR, force_validate builds also returned an error rather than panicking. But the inline comment "a no-op when buffers are already aligned" reads as if alignment is handled in all builds, which is misleading.
To Reproduce
Build with --features force_validate and run the round-trip with an 8-byte-aligned-but-not-16-byte-aligned Decimal128 buffer through from_ffi. consume() returns InvalidArgumentError before realignment can run.
Expected behavior
from_ffi accepts spec-conformant C Data Interface input regardless of force_validate. 8-byte alignment is legal over the C Data Interface; it only becomes an arrow-rs ArrayData invariant violation after import. Today the order is "validate, then realign", which treats protocol-legal input as an invariant violation. The correct order on the import boundary is "realign, then validate".
Proposed fix
Push alignment into ImportedArrowArray::consume (and recursively through consume_child) by replacing ArrayData::new_unchecked with the builder form:
ArrayDataBuilder::new(self.data_type)
.len(len)
.offset(offset)
.null_count(null_count)
.null_bit_buffer(null_bit_buffer)
.buffers(buffers)
.child_data(child_data)
.align_buffers(true)
.skip_validation(true)
.build()?
ArrayDataBuilder::build runs align_buffers before validate_data (arrow-data/src/data.rs:2190-2197), so this works correctly under force_validate while preserving the existing intent of skipping FFI validation on the hot path.
Then:
- Drop the now-redundant outer
data.align_buffers() calls in from_ffi / from_ffi_and_data_type to avoid a duplicate recursive scan.
- Remove the
#[cfg(not(feature = "force_validate"))] gate on test_decimal128_under_aligned_round_trip so the case is covered under that feature too.
It would also be worth tightening the doc on ArrayData::align_buffers to note that on import boundaries it must be called before validation; the current doc only says it's "useful when interacting with buffers produced by other systems" without addressing ordering.
Additional context
Raised in review of #10030: #10030 (review)
Follow-up to #10030 / #10028.
Describe the bug
arrow::ffi::from_ffiandfrom_ffi_and_data_typecalldata.align_buffers()afterImportedArrowArray::consume()to realign protocol-legal but under-aligned C Data Interface buffers (e.g. 8-byte-alignedDecimal128from a JVM producer). Undercfg(feature = "force_validate")this realignment is unreachable:consume()constructsArrayDataviaArrayData::new_unchecked, which goes throughArrayDataBuilder::build. That builder runsvalidate_data()wheneverforce_validateis enabled, regardless ofskip_validation:validate()rejects misalignedBufferSpec::FixedWidthbuffers (arrow-data/src/data.rs:842-867), soconsume()?returnsInvalidArgumentErrorbeforefrom_ffi'salign_buffers()runs. The same applies recursively throughconsume_children/consume_child.The new regression test added in #10030 (
test_decimal128_under_aligned_round_trip) is gated#[cfg(not(feature = "force_validate"))], so CI under that feature does not surface the gap.This is not a regression introduced by #10030 — prior to that PR,
force_validatebuilds also returned an error rather than panicking. But the inline comment "a no-op when buffers are already aligned" reads as if alignment is handled in all builds, which is misleading.To Reproduce
Build with
--features force_validateand run the round-trip with an 8-byte-aligned-but-not-16-byte-alignedDecimal128buffer throughfrom_ffi.consume()returnsInvalidArgumentErrorbefore realignment can run.Expected behavior
from_ffiaccepts spec-conformant C Data Interface input regardless offorce_validate. 8-byte alignment is legal over the C Data Interface; it only becomes an arrow-rsArrayDatainvariant violation after import. Today the order is "validate, then realign", which treats protocol-legal input as an invariant violation. The correct order on the import boundary is "realign, then validate".Proposed fix
Push alignment into
ImportedArrowArray::consume(and recursively throughconsume_child) by replacingArrayData::new_uncheckedwith the builder form:ArrayDataBuilder::buildrunsalign_buffersbeforevalidate_data(arrow-data/src/data.rs:2190-2197), so this works correctly underforce_validatewhile preserving the existing intent of skipping FFI validation on the hot path.Then:
data.align_buffers()calls infrom_ffi/from_ffi_and_data_typeto avoid a duplicate recursive scan.#[cfg(not(feature = "force_validate"))]gate ontest_decimal128_under_aligned_round_tripso the case is covered under that feature too.It would also be worth tightening the doc on
ArrayData::align_buffersto note that on import boundaries it must be called before validation; the current doc only says it's "useful when interacting with buffers produced by other systems" without addressing ordering.Additional context
Raised in review of #10030: #10030 (review)