GH-43427: [C++][Parquet] Deprecate ColumnChunk::file_offset field and no longer write Metadata at end of Chunk#43428
Merged
mapleFU merged 11 commits intoapache:mainfrom Aug 7, 2024
Conversation
|
|
Member
Author
|
Also cc @emkornfield @etseidl |
Member
|
We should not write arrow/cpp/src/parquet/column_writer.cc Line 670 in 62ee676 |
Member
Author
wgtmac
reviewed
Aug 3, 2024
cpp/src/parquet/column_writer.cc
Outdated
| // Write metadata at end of column chunk | ||
| metadata_->WriteTo(sink_.get()); | ||
|
|
||
| // Not write metadata at end of column chunk since we will |
Member
There was a problem hiding this comment.
I'd rather removing these lines to not confuse readers.
cpp/src/parquet/column_writer.cc
Outdated
|
|
||
| // Write metadata at end of column chunk | ||
| metadata_->WriteTo(in_memory_sink_.get()); | ||
| // Not write metadata at end of column chunk since we will |
cpp/src/parquet/metadata.cc
Outdated
Comment on lines
+1540
to
+1542
| // https://github.com/apache/parquet-format/pull/440 | ||
| // The `file_offset` field is deprecated and should be set to 0 for writer | ||
| // if the column chunk has not been written outsidethe footer. |
Member
There was a problem hiding this comment.
Suggested change
| // https://github.com/apache/parquet-format/pull/440 | |
| // The `file_offset` field is deprecated and should be set to 0 for writer | |
| // if the column chunk has not been written outsidethe footer. | |
| // The `file_offset` field is deprecated and should be set to 0. | |
| // See https://github.com/apache/parquet-format/pull/440 for detail. |
cpp/src/parquet/metadata.h
Outdated
| // column chunk | ||
| // Byte offset of `ColumnMetaData` in `file_path()`. | ||
| // | ||
| // Note that the meaning of this field has been inconsistent between implementations |
Member
There was a problem hiding this comment.
Suggested change
| // Note that the meaning of this field has been inconsistent between implementations | |
| // Note that the meaning of this field has been inconsistent among implementations |
etseidl
approved these changes
Aug 3, 2024
cpp/src/parquet/metadata.h
Outdated
| // Byte offset of `ColumnMetaData` in `file_path()`. | ||
| // | ||
| // Note that the meaning of this field has been inconsistent between implementations | ||
| // so its use has since been deprecated in the Parquet specification. Moder |
Contributor
There was a problem hiding this comment.
Suggested change
| // so its use has since been deprecated in the Parquet specification. Moder | |
| // so its use has since been deprecated in the Parquet specification. Modern |
cpp/src/parquet/metadata.cc
Outdated
| } | ||
| // https://github.com/apache/parquet-format/pull/440 | ||
| // The `file_offset` field is deprecated and should be set to 0 for writer | ||
| // if the column chunk has not been written outsidethe footer. |
Contributor
There was a problem hiding this comment.
Suggested change
| // if the column chunk has not been written outsidethe footer. | |
| // if the column chunk has not been written outside the footer. |
wgtmac
approved these changes
Aug 3, 2024
Member
Author
|
Would fix this later: |
Member
Author
|
Failed CI is unrelated. Will merge if no more negative comment in August 6th |
Member
Author
|
Merge because collect 1 approve |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 9b58454. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 31 possible false positives for unstable benchmarks that are known to sometimes produce them. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Rationale for this change
See github issue
What changes are included in this PR?
Force
ColumnChunk::file_offsettobe 0Are these changes tested?
No
Are there any user-facing changes?
Might affect user using legacy
ColumnChunk::file_offset