Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally require alignment when reading IPC, respect alignment when writing #5554

Merged
merged 14 commits into from Apr 4, 2024

Conversation

hzuo
Copy link
Contributor

@hzuo hzuo commented Mar 26, 2024

Which issue does this PR close?

Closes #5553

Rationale for this change

FileWriter and StreamWriter should ensure that the data is written with appropriate alignment such that arrays can be used without copying to a more-aligned buffer.

In particular, as of Rust 1.77.0 and LLVM 18, i128 now has a 16-byte alignment requirement even on x86 (ARM always had this requirement), i.e. std::mem::align_of::<i128> == 16. So Decimal128Arrays must be aligned to a 16-byte boundary when serialized into an IPC buffer. The pad_to_8 used everywhere in the IPC code causes it to pad insufficiently.

This prevents readers of the IPC data generated by this crate from doing true zero-copy reads (e.g. mmapping) since the data may be insufficiently aligned. See #5165, #5153.

On some platforms, SIMD may also be significantly slower if the beginning of the IPC block isn't aligned to a 16-, 32-, or 64- byte boundary (as discussed in the Arrow spec document).

(copied from issue)

What changes are included in this PR?

  • Make all the IPC code actually respect the alignment specified in IpcWriteOptions
  • Add new require_alignment option to FileDecoder, read_record_batch, and read_dictionary, which enforces that the read is a zero-copy read. Previously if the buffer was unaligned then the code automatically allocates an aligned buffer and performs a memcpy (build_aligned => align_buffers), which users of Arrow as a zero-copy format may not expect / may want to enforce does not happen.

Are there any user-facing changes?

Yes:

  • alignment in IpcWriteOptions::try_with cannot be larger than 64 bytes - previously unbounded (breaking but minor)
  • New require_alignment argument on read_record_batch and read_dictionary (breaking but minor)
  • New option with_require_alignment to FileDecoder (non-breaking)

@github-actions github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Mar 26, 2024
@hzuo hzuo changed the title fix Respect alignment requirements when writing IPC data Mar 26, 2024

#[test]
fn test_decimal128_alignment8_is_unaligned() {
const IPC_ALIGNMENT: u8 = 8;
Copy link
Contributor Author

@hzuo hzuo Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is equivalent to the current behavior without the changes in this PR, it's equivalent to using pad_to_8 everywhere.

Because the Decimal128Array is insufficiently aligned, this prevents readers from doing a true zero-copy read (e.g. mmapping), because they need to copy the block into a more-aligned buffer.

}

/// Calculate an 8-byte boundary and return the number of bytes needed to pad to 8 bytes
const PADDING: [u8; 64] = [0; 64];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously a Vec was being allocated every time padding was needed

@tustvold
Copy link
Contributor

I'm travelling for the next few days, but will try to get to this on my return if nobody else gets there first

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this contribution @Huzo 🙏

I think it looks great overall and makes sense. I had a few small comments but I also think this PR could be merged as is.

It would probably be good to get someone else like @tustvold t give it a review prior to merge, but all in all very nice

arrow-ipc/src/reader.rs Outdated Show resolved Hide resolved
arrow-ipc/src/reader.rs Outdated Show resolved Hide resolved
Comment on lines 691 to 692
/// Specify whether to enforce zero copy, which means the array data inside the record batches
/// produced by this decoder will always reference the input buffer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about calling this concept / field require_alignment which describes what is required (rather than how it is enforced)?

It might also help make the intent clearer to users (that the IPC decoder will error if the buffers are not aligned properly)

I don't feel super strongly about this, but I was a little confused about what enforce_zero_copy meant until I found this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, will make this change.

My original thinking was that there might be a variety of reasons why IPC decoders may need to create a copy of the input data, and this would be a catch-all so that the users don't have to understand the intricacies of why or why not copies need to be made.

But agree that it ends up being kinda vague, and in the future if there are other reasons copies need to be made, probably better to expose each of the "reasons" as a granular "requirement" - if there ends up being a ton, we can expose a catch-all, but no reason to anticipate that today at the expense of clarity.

arrow-ipc/src/reader.rs Show resolved Hide resolved
arrow-ipc/src/reader.rs Outdated Show resolved Hide resolved
arrow-ipc/src/writer.rs Outdated Show resolved Hide resolved
arrow-ipc/src/writer.rs Outdated Show resolved Hide resolved
arrow-ipc/src/writer.rs Outdated Show resolved Hide resolved
arrow-ipc/src/writer.rs Show resolved Hide resolved
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️
Thanks again @hzuo

@alamb
Copy link
Contributor

alamb commented Apr 2, 2024

so close!

@alamb alamb changed the title Respect alignment requirements when writing IPC data Optionally require alignment requirements when reading IPC, respect alignment when writing Apr 2, 2024
@tustvold tustvold added the api-change Changes to the arrow API label Apr 3, 2024
Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, my only concern is the breaking API change. We have had some community feedback requesting we reduce the frequency of these - #5368

arrow-ipc/src/reader.rs Show resolved Hide resolved
@hzuo hzuo changed the title Optionally require alignment requirements when reading IPC, respect alignment when writing Optionally require alignment when reading IPC, respect alignment when writing Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPC code writes data with insufficient alignment
3 participants