Validate ArrayData Buffer Alignment and Automatically Align IPC buffers (#4255) #4681

tustvold · 2023-08-11T11:00:40Z

Which issue does this PR close?

Rationale for this change

This will allow zero-copy IPC (#4254), whilst also acting as an escape valve for issues like apache/arrow#32276 where arrow-cpp produces unaligned buffers

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-08-11T11:03:52Z

arrow-ipc/src/reader.rs

        }
        t => unreachable!("Data type {:?} either unsupported or not primitive", t),
    };

    Ok(make_array(array_data))
 }

-/// Checks if given `Buffer` is properly aligned with `T`.
-/// If not, copying the data and padded it for alignment.
-fn get_aligned_buffer<T>(buffer: &Buffer, length: usize) -> Buffer {


This was originally added because aarch64 has stronger alignment requirements for i128 than the 64-bit alignment commonly provided by flatbuffers, now this is handled systematically for all types

tustvold · 2023-08-11T11:04:19Z

arrow-data/src/data.rs


                    if buffer.len() < min_buffer_size {
                        return Err(ArrowError::InvalidArgumentError(format!(
                            "Need at least {} bytes in buffers[{}] in array of type {:?}, but got {}",
                            min_buffer_size, i, self.data_type, buffer.len()
                        )));
                    }
+
+                    if buffer.as_ptr().align_offset(*alignment) != 0 {


We now actually verify this as part of ArrayData, instead of panicking when you try to create an array 🎉

tustvold · 2023-08-11T11:05:07Z

arrow-data/src/data.rs

-            buffers: vec![BufferSpec::FixedWidth { byte_width }],
+            buffers: vec![BufferSpec::FixedWidth {
+                byte_width: mem::size_of::<T>(),
+                alignment: mem::align_of::<T>(),


One consequence of this formulation is the alignment is architecture specific, I suspect this is fine, but it potentially worth highlighting.

Well let's hope nobody mmaps arrow data between different architectures w/o carefully thinking about alignment.

On that note: I wonder what the cost would be to heavily overalign buffers in general to to make them valid on all supported architectures. Like why not just align everything to 256bits?

Like why not just align everything to 256bits

We do, Rust doesn't 😄 And at least until the allocator API is stabilised there is no way to over-align a Vec

I don't understand why we are aligning all buffers to native sizes -- I thought the point was that the arrow spec said we should align buffers to 64 bits -- if that is the case, shouldn't this code reflect the spec, not the native size?

Because native alignment is what the arrow-rs implementation requires, for many types this is less than 64-bits. We don't, for example, care if the values buffer of a StringArray is aligned at all. On the flip side, some architectures, such as aarch64 require more than 64-bit alignment for i128, and so we require that also

I think the documentation added in ff6af65 make the behavior in this PR much clearer.

Thank you

tustvold · 2023-08-11T11:08:41Z

arrow-array/src/array/list_array.rs

+        let array_data = unsafe {
+            ArrayData::builder(DataType::Int32)
+                .add_buffer(buf2)
+                .build_unchecked()


We have to use build_unchecked here, as ArrayData will now complain about the alignment 🎉

crepererum · 2023-08-11T11:11:19Z

arrow-data/src/data.rs

+
+                    if buffer.as_ptr().align_offset(*alignment) != 0 {
+                        return Err(ArrowError::InvalidArgumentError(format!(
+                            "Misaligned buffers[{i}] in array of type {:?}, expected {alignment}",


I think it would be helpful for debugging to include the actual alignment in addition to the expected value.

alamb · 2023-08-11T11:14:57Z

It appears something similar was done in arrow2 recently: jorgecarleitao/arrow2#1535

crepererum · 2023-08-11T11:17:08Z

arrow-data/src/data.rs

+        for (buffer, spec) in self.buffers.iter_mut().zip(&layout.buffers) {
+            if let BufferSpec::FixedWidth { alignment, .. } = spec {
+                if buffer.as_ptr().align_offset(*alignment) != 0 {
+                    *buffer = Buffer::from_slice_ref(buffer.as_ref())


should we maybe log a debug message when this happens? Some users might find it confusing that the buffers are silently copied / re-aligned.

We don't currently have a logging setup #1495

Or do we consider to use a feature for automatic alignment?

The way I read this PR, if a user is creating ArrayData directly, they have to explicitly call align_buffer in order to ensure the data is aligned and the documentation says it may copy the data.

If they are using the IPC reader then the reader may automatically align (by copying) the data, which seems better than erroring.

As a user, if I didn't want buffers realigned automatically, it would make probably want a setting on the IPC reader that said "error rather than silently fix" on unaligned data. That seems like something we could add as a follow on PR if someone wants that behavior

tustvold · 2023-08-11T11:23:15Z

t appears something similar was done in arrow2 recently: jorgecarleitao/arrow2#1535

That is for FFI, which I don't intend to make automatic, as it would likely be very surprising for users. Instead the fact we provide an escape valve in the form of ArrayData::align_buffers I think is sufficient

tustvold · 2023-08-11T11:28:34Z

arrow/tests/array_validation.rs

@@ -56,7 +56,9 @@ fn test_bad_number_of_buffers() {
 }

 #[test]
-#[should_panic(expected = "integer overflow computing min buffer size")]
+#[should_panic(
+    expected = "Need at least 18446744073709551615 bytes in buffers[0] in array of type Int64, but got 8"


As an allocation cannot be larger than isize, we might as well just saturate at usize::MAX, we're going to fail anyway

viirya · 2023-08-11T17:22:33Z

arrow-data/src/data.rs

+    ///
+    /// This can be useful for when interacting with data sent over IPC or FFI, that may
+    /// not meet the minimum alignment requirements
+    pub fn align_buffers(&mut self) {


Hmm, so can we use this to address unaligned imported arrays issue like #4431?

Nice. It seems that it might take longer for a fix at Java Arrow implementation.

Could we also please document clearly

What minimum alignment requirements are meant (is it Rust? Is it Arrow?)

What happens if we have "unaligned buffers"? (Rust errors / panics? Less performance?)

alamb

This PR looks good to me. I am also going to run the tests on an ARM processor to verify it is ok. Given the choice between copying to realign and runtime error, I think copying to realign is a reasonable behavior change

As I mentioned previously, it may be useful to add a "error rather than copy mode" but we can do that if anyone else thinks that is valuable

I think the documentation could be more clear about what alignment we are referring to and what happens when data is not aligned.

But overall thank you @tustvold -- I think this is a significant step forward

alamb · 2023-08-15T11:16:53Z

arrow-data/src/data.rs

+        for (buffer, spec) in self.buffers.iter_mut().zip(&layout.buffers) {
+            if let BufferSpec::FixedWidth { alignment, .. } = spec {
+                if buffer.as_ptr().align_offset(*alignment) != 0 {
+                    *buffer = Buffer::from_slice_ref(buffer.as_ref())


The way I read this PR, if a user is creating ArrayData directly, they have to explicitly call align_buffer in order to ensure the data is aligned and the documentation says it may copy the data.

If they are using the IPC reader then the reader may automatically align (by copying) the data, which seems better than erroring.

As a user, if I didn't want buffers realigned automatically, it would make probably want a setting on the IPC reader that said "error rather than silently fix" on unaligned data. That seems like something we could add as a follow on PR if someone wants that behavior

alamb · 2023-08-15T11:20:02Z

arrow-data/src/data.rs

@@ -1629,7 +1656,7 @@ impl DataTypeLayout {
 #[derive(Debug, PartialEq, Eq)]
 pub enum BufferSpec {
    /// each element has a fixed width
-    FixedWidth { byte_width: usize },
+    FixedWidth { byte_width: usize, alignment: usize },


I think we should document here what alignment represents (e.g. required pointer alignment for buffers of this type)

alamb · 2023-08-15T11:21:35Z

arrow-data/src/data.rs

-            buffers: vec![BufferSpec::FixedWidth { byte_width }],
+            buffers: vec![BufferSpec::FixedWidth {
+                byte_width: mem::size_of::<T>(),
+                alignment: mem::align_of::<T>(),


I don't understand why we are aligning all buffers to native sizes -- I thought the point was that the arrow spec said we should align buffers to 64 bits -- if that is the case, shouldn't this code reflect the spec, not the native size?

alamb · 2023-08-15T11:27:39Z

arrow-data/src/data.rs

+    ///
+    /// This can be useful for when interacting with data sent over IPC or FFI, that may
+    /// not meet the minimum alignment requirements
+    pub fn align_buffers(&mut self) {


Could we also please document clearly

What minimum alignment requirements are meant (is it Rust? Is it Arrow?)

What happens if we have "unaligned buffers"? (Rust errors / panics? Less performance?)

alamb · 2023-08-15T11:33:30Z

arrow-ipc/src/reader.rs

@@ -1704,4 +1648,40 @@ mod tests {
        let output_batch = roundtrip_ipc_stream(&input_batch);
        assert_eq!(input_batch, output_batch);
    }
+
+    #[test]
+    fn test_unaligned() {


I ran this test without the changes in the PR and it fails like this:

---- reader::tests::test_unaligned stdout ---- thread 'reader::tests::test_unaligned' panicked at 'Memory pointer is not aligned with the specified scalar type', /Users/alamb/Software/arrow-rs2/arrow-buffer/src/buffer/scalar.rs:125:42 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

alamb

I also verified this PR and all tests pass on an ARM processor ("Ampere Altra" on GCP) 👍

tustvold · 2023-08-16T09:57:58Z

I've added a number of additional doc comments to hopefully clarify what alignment means in this context

Automatically align misaligned IPC buffers (apache#4255)

7b12357

tustvold added the api-change Changes to the arrow API label Aug 11, 2023

github-actions bot added the arrow Changes to the arrow crate label Aug 11, 2023

tustvold commented Aug 11, 2023

View reviewed changes

Update test

aa57307

tustvold commented Aug 11, 2023

View reviewed changes

tustvold changed the title ~~Automatically align misaligned IPC buffers (#4255)~~ Validate ArrayData Bufer Alignment and Automatically Align IPC buffers (#4255) Aug 11, 2023

tustvold changed the title ~~Validate ArrayData Bufer Alignment and Automatically Align IPC buffers (#4255)~~ Validate ArrayData Buffer Alignment and Automatically Align IPC buffers (#4255) Aug 11, 2023

crepererum reviewed Aug 11, 2023

View reviewed changes

Further test fix

5d3a79d

Format

6eddf1f

tustvold commented Aug 11, 2023

View reviewed changes

Review feedback

bb03bd7

viirya reviewed Aug 11, 2023

View reviewed changes

tustvold requested a review from alamb August 15, 2023 07:49

alamb approved these changes Aug 15, 2023

View reviewed changes

alamb reviewed Aug 15, 2023

View reviewed changes

More docs

ff6af65

tustvold merged commit 197c425 into apache:master Aug 16, 2023

tustvold mentioned this pull request Sep 7, 2023

Add StringViewArray and BinaryViewArray (#4253) #4585

Closed

tustvold mentioned this pull request Sep 25, 2023

Buffer from external memory doesn't match ScalarBuffer alignment requirement #4431

Closed

viirya mentioned this pull request Sep 26, 2023

Make align_buffers as public API #4863

Merged

EnricoMi mentioned this pull request Oct 1, 2024

GH-32276: [C++][FlightRPC] Align RecordBatch buffers given to IPC apache/arrow#44279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate ArrayData Buffer Alignment and Automatically Align IPC buffers (#4255) #4681

Validate ArrayData Buffer Alignment and Automatically Align IPC buffers (#4255) #4681

tustvold commented Aug 11, 2023

tustvold Aug 11, 2023

tustvold Aug 11, 2023

tustvold Aug 11, 2023

crepererum Aug 11, 2023

tustvold Aug 11, 2023 •

edited

Loading

alamb Aug 15, 2023

tustvold Aug 15, 2023

alamb Aug 17, 2023

tustvold Aug 11, 2023

crepererum Aug 11, 2023

alamb commented Aug 11, 2023

crepererum Aug 11, 2023

tustvold Aug 11, 2023

viirya Aug 11, 2023

alamb Aug 15, 2023

tustvold commented Aug 11, 2023

tustvold Aug 11, 2023

viirya Aug 11, 2023

tustvold Aug 11, 2023

viirya Aug 11, 2023

alamb Aug 15, 2023

alamb left a comment

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb Aug 15, 2023

alamb left a comment •

edited

Loading

tustvold commented Aug 16, 2023

Validate ArrayData Buffer Alignment and Automatically Align IPC buffers (#4255) #4681

Validate ArrayData Buffer Alignment and Automatically Align IPC buffers (#4255) #4681

Conversation

tustvold commented Aug 11, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Aug 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment • edited Loading

Choose a reason for hiding this comment

tustvold commented Aug 16, 2023

tustvold Aug 11, 2023 •

edited

Loading

alamb left a comment •

edited

Loading