Work around incompatibilities between V2 page header handling and zStandard compression in Parquet writer #14772
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
etseidl
changed the title
|
This PR is a temporary fix that should be in 24.02. A proper fix will be ready for 24.04. |
vuule
added
bug
vuule
requested changes
Jan 18, 2024
cpp/src/io/parquet/writer_impl.cu
Outdated
| @@ -2184,6 +2184,9 @@ writer::impl::impl(std::vector<std::unique_ptr<data_sink>> sinks, | |||
| if (options.get_metadata()) { | |||
| _table_meta = std::make_unique<table_input_metadata>(*options.get_metadata()); | |||
| } | |||
| if (_write_v2_headers and _compression == Compression::ZSTD) { | |||
| CUDF_FAIL("V2 page headers cannot be used with ZSTD compression"); | |||
There was a problem hiding this comment.
we need the same check for the chunked case as well
There was a problem hiding this comment.
Oops. Or should the test (and the lines above it) go in init_state() instead? (And maybe change the name of init_state).
There was a problem hiding this comment.
Looks like I'm late with the answer:)
Side note: the current code in init_state should not be there (breaks the strong exception guarantee)
Co-authored-by: Vukasin Milovanovic <vmilovanovic@nvidia.com>
|
/ok to test |
|
/ok to test |
vuule
approved these changes
Jan 18, 2024
vuule
reviewed
Jan 18, 2024
|
/ok to test |
davidwendt
approved these changes
Jan 21, 2024
|
/ok to test |
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In the version 2 Parquet page header, neither the repetition nor definition level data is compressed. The current Parquet writer achieves this by offsetting the input buffers passed to nvcomp to skip this level data. Doing so can lead to mis-aligned data being passed to nvcomp (for zstd, input currently must be aligned on a 4 byte boundary). This PR is a short-term fix that will print an error and exit if zStandard compression is used with V2 page headers. This also fixes an underestimation of the maximum V2 page header size.
Checklist