fix(arrow-array): align FFI buffers before validation under force_validate

[bit2swaz]

Which issue does this PR close?

• Closes from_ffi realignment is bypassed under force_validate #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.

[Jefffrey]

how is this change tested if the test already passes on main?

[bit2swaz]

it passes on main because arrow-array's force_validateis empty and doesnt turn on the validation that lives in arrow-data:

arrow-rs/arrow-array/Cargo.toml

Line 72 in d6b0bd7

force_validate = []

so in the arrow-array test job the misaligned buffer is never rejected, with or without this change. the validation only actually runs when arrow-data/force_validate is on which only happens thru the arrow meta-crate's force_validate (it forwards to both crates) and that job doesnt run arrow-array's unit tests. so factually speaking today no CI config runs this test with the validation enabled

and being honest with you: i really couldnt find a way to write a test that exercises the fix under arrow-data/force_validate. the test has to hand from_ffi a buffer thats under aligned for its type. the only way to get one is an ArrayData from build_unchecked but under force_validate that validates anyway (the gate ignores skip_validation), so the fixture is rejected at construction, before from_ffi runs

arrow-rs/arrow-data/src/data.rs

Lines 2267 to 2268 in d6b0bd7

	if !skip_validation.get() || cfg!(feature = "force_validate") {
	data.validate_data()?;

you cant source it from a valid array either: to be under aligned for the import type youd need a source type with the same byte width but smaller alignment and no arrow primitive fits that (align_of == size_of). so the misaligned input is genuinely unconstructable in a force_validate build

what is covered: normal builds run the realignment test. under arrow-data/force_validate the full ffi test module still passes since valid arrays go through the same align-before-validate path in consume so the reorder doesnt break anything. the one gap is the under aligned case under force_validate which is the case that cant be built based on my research

so i can realistically think of two options: leave it as is with the gate and the comment explaining why, or i hand build the FFI_ArrowArray from a raw under aligned pointer in the test to bypass ArrayData entirely and get real coverage, but at the cost of more unsafe test code.

which would you prefer?

[Jefffrey]

or i hand build the FFI_ArrowArray from a raw under aligned pointer in the test to bypass ArrayData entirely and get real coverage, but at the cost of more unsafe test code.

lets see how this pans out; i think it would just need creating a decimal128 array, then manually offsetting the pointer by 8 to unalign it (ensuring still stays within allocation bound) 🤔

[bit2swaz]

done. now the fixture is a valid FixedSizeBinary(16) array (1-byte alignment so its okay under force_validate) over the 8-byte-offset buffer imported as Decimal128. now ungated and red without the fix

[Jefffrey]

done. now the fixture is a valid FixedSizeBinary(16) array (1-byte alignment so its okay under force_validate) over the 8-byte-offset buffer imported as Decimal128. now ungated and red without the fix

could you double check this? i tried running the test on main (without the accompanying fix) and it still succeeds, both with force_validate enabled and with no default features

[bit2swaz]

checked it out. thats the old build_unchecked test still on main which passes without the fix (the outer align_buffers() hides it and its gated off under force_validate). the new FixedSizeBinary(16) version in this PR is red w/o the fix in all of ffi, force_validate,ffi, and ffi,arrow-data/force_validate

[Jefffrey]

ah i probably wasnt running with the ffi feature, my bad

[Jefffrey]

i tried again; copying only the test_decimal128_under_aligned_round_trip test into main without any of these fixes, and running with ffi + force_validate (and without), the test succeeds on main:

arrow-rs (main)$ cargo test -p arrow-array --features ffi --lib under_aligned
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.06s
     Running unittests src/lib.rs (/Users/jeffrey/.cargo_target_cache/debug/deps/arrow_array-6904f486d1ee4d68)

running 1 test
test ffi::tests_to_then_from_ffi::test_decimal128_under_aligned_round_trip ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 770 filtered out; finished in 0.00s

arrow-rs (main)$ cargo test -p arrow-array --features force_validate,ffi --lib under_aligned
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.07s
     Running unittests src/lib.rs (/Users/jeffrey/.cargo_target_cache/debug/deps/arrow_array-a9af9d70cecd6150)

running 1 test
test ffi::tests_to_then_from_ffi::test_decimal128_under_aligned_round_trip ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 756 filtered out; finished in 0.01s

could you double check if this test is exercising a failure behaviour we're supposed to see on main? or am i doing something wrong

for reference, this is the copy pasted test as of the state of this PR:

// arrow-array/src/ffi.rs

#[test]
fn test_decimal128_under_aligned_round_trip() -> Result<()> {
    // 8-byte-aligned i128 buffer: legal over the C Data Interface, but
    // under-aligned for arrow-rs. Carried as `FixedSizeBinary(16)` (1-byte
    // alignment) so the fixture stays valid under `force_validate`, then
    // imported as `Decimal128`, which must realign it.
    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);

    let data = ArrayData::builder(DataType::FixedSizeBinary(16))
        .len(2)
        .add_buffer(under_aligned)
        .build()?;

    let array = FFI_ArrowArray::new(&data);
    let imported = unsafe { from_ffi_and_data_type(array, DataType::Decimal128(10, 2)) }?;
    let array = Decimal128Array::from(imported);

    // [0i128, 1, 2] sliced 8 bytes in yields `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);
    Ok(())
}

[bit2swaz]

the test passes on arrow-array --features force_validate because arrow-array's force_validate feature is empty and doesnt forward to arrow-data. so cfg!(feature = "force_validate") inside arrow-data::ArrayDataBuilder::build() is false, validation is skipped, and main's outer align_buffers() fixes the misalignment before anything reads it

the failure only shows up under cargo test -p arrow --features force_validate,ffi where arrow's force_validate does forward to arrow-data. thats why the test is in arrow/tests/ffi.rs as its the only place where the ffi feature and arrow-data/force_validate are both active

[Jefffrey]

the test passes on arrow-array --features force_validate because arrow-array's force_validate feature is empty and doesnt forward to arrow-data

this seems like a bug and something we should fix, especially as it seems the original issue was assuming that force_validate would include the arrow-data one

[bit2swaz]

checks out. from what i can see, adding "arrow-data/force_validate" to arrow-array's feature is the right fix but then it breaks 5 tests in arrow-array that use build_unchecked to construct invalid arrays and validate afterward as the forward makes build_unchecked panic first with arrow-data's error before those tests reach their own checks

i can file a follow up issue for it if that works

[Jefffrey]

i think it would be preferable to fix the feature issue (make arrow-array force_validate enable arrow-data one) as that gap seems unintuitive, and then keep the test in ffi so it stays close to the relevant code

it seems force_validate was added to arrow-array to satisfy a clippy lint, so im assuming it was just a miss that it didnt also enable arrow-data's force_validate:

• Fix Clippy for the Rust 1.80 release #6116

[bit2swaz]

done and pushed. arrow-array's force_validate now forwards to arrow-data and the test is back in arrow-array/src/ffi.rs. the forward broke 5 tests that relied on build_unchecked skipping validation, so i routed RunArray::try_new through build()? and dropped one stale should_panic string. green in both configs. ready for review again :)

[Jefffrey]

thanks for persisting with this; almost there, after fixing some merge conflicts

[bit2swaz]

done, merged main and pushed

#10807 removed the ArrayData path in RunArray::try_new so my change there is gone. the fixed_size_binary test is back to the old expected string, gated under not(force_validate) so it tests the FixedSizeBinaryArray::from panic and not the arraydata one

ready for review again :)