[I] `from_ffi` realignment is bypassed under `force_validate` [arrow-rs]

["mbutrovich (via GitHub)" <gi...@apache.org>]

mbutrovich opened a new issue, #10034:
URL: https://github.com/apache/arrow-rs/issues/10034

   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`:
   
   ```rust
   // 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:
   
   ```rust
   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: https://github.com/apache/arrow-rs/pull/10030#pullrequestreview-4383084253
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] `from_ffi` realignment is bypassed under `force_validate` [arrow-rs]

["bit2swaz (via GitHub)" <gi...@apache.org>]

bit2swaz commented on issue #10034:
URL: https://github.com/apache/arrow-rs/issues/10034#issuecomment-5381541318

   im taking this. ill push the align into `consume()`'s builder so it runs before the `force_validate`-gated validate (same as the ipc reader) and drop the now-redundant outer `align_buffers()`. brb


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] `from_ffi` realignment is bypassed under `force_validate` [arrow-rs]

["Jefffrey (via GitHub)" <gi...@apache.org>]

Jefffrey closed issue #10034: `from_ffi` realignment is bypassed under `force_validate`
URL: https://github.com/apache/arrow-rs/issues/10034


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org