Jefffrey commented on code in PR #12010:
URL: https://github.com/apache/arrow-rs/pull/12010#discussion_r4477359059
##########
arrow-array/src/ffi.rs:
##########
@@ -1778,6 +1786,74 @@ mod tests_from_ffi {
Ok(())
}
+ /// Offsets that a producer stores at an address not aligned for their
type must
+ /// import, as the C data interface only recommends alignment.
+ ///
+ /// <https://github.com/apache/arrow-rs/issues/7136>
+ #[test]
+ fn test_bytes_with_unaligned_offsets() -> Result<()> {
+ // The C layout of `FFI_ArrowArray`, filled in by hand as a C producer
would.
+ #[repr(C)]
+ struct RawArrowArray {
Review Comment:
we can just gate the test under `not(force_validate)` since CI should still
run with & without force validate for arrow-array; it should make the test
setup simpler that way, using arraydata instead of redefining the C data
interface as a struct there
##########
arrow-array/src/ffi.rs:
##########
@@ -389,32 +389,28 @@ impl ImportedArrowArray<'_> {
let buffer_begin = can_contain_null_mask as usize;
let buffer_end = self.array.num_buffers() - usize::from(variadic);
- let variadic_buffer_lens = if variadic {
+ let variadic_buffer_lens: Vec<i64> = if variadic {
// Each views array has 1 (optional) null buffer, 1 views buffer,
1 lengths buffer.
// Rest are variadic.
let num_variadic_buffers =
self.array.num_buffers() - (2 +
usize::from(can_contain_null_mask));
- if num_variadic_buffers == 0 {
- &[]
- } else {
- let lengths = self.array.buffer(self.array.num_buffers() - 1);
- // SAFETY: is lengths is non-null, then it must be valid for
up to num_variadic_buffers.
- // The C data interface requires buffers to be aligned for
their type.
- #[expect(
- clippy::cast_ptr_alignment,
- reason = "the C data interface requires aligned buffers"
- )]
- unsafe {
- std::slice::from_raw_parts(lengths.cast::<i64>(),
num_variadic_buffers)
- }
- }
+ // the pointer may not be aligned for `i64`, as `consume` aligns
buffers only later.
+ #[expect(clippy::cast_ptr_alignment)]
+ let lengths = self
+ .array
+ .buffer(self.array.num_buffers() - 1)
+ .cast::<i64>();
+ // SAFETY: if lengths is non-null, then it must be valid for up to
num_variadic_buffers.
+ (0..num_variadic_buffers)
+ .map(|i| unsafe { lengths.add(i).read_unaligned() })
Review Comment:
it might be better to use a `Cow` here, to avoid allocation in the aligned
case; then we only copy to aligned buffer for unaligned case
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
