GH-51303: [C++][Parquet] Update parquet.thrift to sync with 2.14.0 - #51304
GH-51303: [C++][Parquet] Update parquet.thrift to sync with 2.14.0#51304HuaHuaY wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
ALP collides with the internal UNDEFINED encoding sentinel, preventing correct conversion and round-tripping.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Synchronizes Parquet’s C++ Thrift schema and generated bindings with format 2.14.0.
Changes:
- Adds FILE logical type, ALP encoding, and INT96 timestamp ordering.
- Regenerates C++ Thrift serialization and type-support sources.
File summaries
| File | Summary |
|---|---|
cpp/src/parquet/parquet.thrift |
Defines Parquet 2.14.0 schema additions. |
cpp/src/generated/parquet_types.tcc |
Adds generated serialization logic. |
cpp/src/generated/parquet_types.h |
Adds generated declarations and enum values. |
cpp/src/generated/parquet_types.cpp |
Adds generated implementations and enum mappings. |
Review details
- Files reviewed: 1/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
893c100 to
4112561
Compare
There was a problem hiding this comment.
🟡 Changes recommended
EncodingToString lacks a mapping for Encoding::ALP.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/5 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Add the missing Encoding::ALP string mapping and regression test.
Review details
Suppressed comments (1)
cpp/src/parquet/types.h:542
ALPis now a valid public encoding, butEncodingToString()still has nocase Encoding::ALP(cpp/src/parquet/types.cc:250-272). As a result, printer output and diagnostics for ALP metadata reportUNKNOWNinstead ofALP; please add the mapping (and a regression test) with this enum update.
ALP = 10,
- Files reviewed: 2/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
4112561 to
d95200d
Compare
|
I reverted the changes to |
There was a problem hiding this comment.
🟡 Changes recommended
The ALP enum-value conflict causes incorrect metadata conversion and requires updates to the mirrored enum, sentinel, string conversion, and tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/4 changed files
- Comments generated: 1
- Review effort level: Lite
| bit-packing; values that cannot be converted losslessly are stored as | ||
| exceptions. See Encodings.md for the detailed specification. | ||
| */ | ||
| ALP = 10; |
There was a problem hiding this comment.
I think this is acceptable.
Rationale for this change
Apache Parquet has just released its format version 2.14.0, which supports ALP, FILE type and some other features. We need to update parquet.thrift and its generated source files to be in sync.
What changes are included in this PR?
Are these changes tested?
Yes.
Are there any user-facing changes?
No.