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

ARROW-8671: [C++] Use new BodyCompression Flatbuffers member for IPC compression metadata #7571

Closed
wants to merge 1 commit into from

Conversation

wesm
Copy link
Member

@wesm wesm commented Jun 28, 2020

If the message uses V4 metadata then we also look for the "ARROW:experimental_compression" field in Message::custom_metadata so that IPC message written with 0.17.x can be read in 1.0.0 and beyond.

@github-actions
Copy link

@wesm
Copy link
Member Author

wesm commented Jun 28, 2020

I confirmed that I can read compressed files (including compressed dictionaries) generated from master

@wesm wesm closed this in f589370 Jun 28, 2020
@wesm wesm deleted the ARROW-8671 branch June 28, 2020 22:22
@wesm wesm restored the ARROW-8671 branch June 28, 2020 22:23
@wesm
Copy link
Member Author

wesm commented Jun 28, 2020

Oops, I didn't mean to merge this patch, sorry! Please review it and I will address any code reviews as follow up

@wesm wesm reopened this Jun 28, 2020
@wesm
Copy link
Member Author

wesm commented Jun 29, 2020

This has an ASAN/UBSAN failure. I will fix within an hour

@wesm
Copy link
Member Author

wesm commented Jun 29, 2020

@pitrou this is already merged (by accident actually, mistyped the PR number on the command line and went too fast), but let me know if you see anything concerning from a fuzz perspective or otherwise. I fixed one fuzz issue already in 76c3e4a

@@ -66,6 +67,10 @@ struct ARROW_EXPORT IpcWriteOptions {
/// like compression
bool use_threads = true;

/// \brief Format version to use for IPC messages and their
/// metadata. Presently using V4 version (readable by v0.8.0 and later).
MetadataVersion metadata_version = MetadataVersion::V4;
Copy link
Member

Choose a reason for hiding this comment

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

I find it a bit weird to expose this an option. Will we be able to write data compatible with other metadata versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This is what I've been saying in the V4/V5 MetadataVersion discussion e-mail

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I had misunderstood that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wesm
Copy link
Member Author

wesm commented Jun 29, 2020

I'll close this for now. Please leave any review comments and I can address them later

@wesm wesm closed this Jun 29, 2020
sgnkc pushed a commit to sgnkc/arrow that referenced this pull request Jul 2, 2020
…compression metadata

If the message uses V4 metadata then we also look for the "ARROW:experimental_compression" field in Message::custom_metadata so that IPC message written with 0.17.x can be read in 1.0.0 and beyond.

Closes apache#7571 from wesm/ARROW-8671

Authored-by: Wes McKinney <[email protected]>
Signed-off-by: Wes McKinney <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants