Conversation
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
|
|
|
cc @brkyvz |
51cd48e to
77011b6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There are release-build UB/crash risks in the new FILE extension restoration path due to unchecked checked_cast assumptions and potential null dereferences.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces support for Parquet’s FILE logical type in the C++ Parquet implementation and adds a temporary Arrow extension type (parquet.file.experimental.v1) to enable read/write round-trips while Arrow lacks a native spec-level type for FILE.
Changes:
- Add
FILEas a Parquet logical type (LogicalType::File()/FileLogicalType) with thrift serialization support. - Implement Arrow<->Parquet schema conversion for
FILEvia a new Arrow extension type and ensure column-pruning behavior returns a struct when only part of the storage is read. - Update
parquet.thriftand regenerated thrift outputs; add unit tests covering schema conversion and read/write round-trips.
File summaries
| File | Description |
|---|---|
| cpp/src/parquet/types.h | Adds FILE logical type API and FileLogicalType declaration. |
| cpp/src/parquet/types.cc | Implements FILE logical type behavior, thrift conversion, and nesting classification. |
| cpp/src/parquet/schema_test.cc | Extends logical type tests for FILE creation/properties/roundtrip. |
| cpp/src/parquet/parquet.thrift | Adds FileType to LogicalType union (and other upstream thrift updates). |
| cpp/src/parquet/arrow/schema.cc | Adds Arrow extension mapping to/from Parquet FILE; metadata restoration tweaks. |
| cpp/src/parquet/arrow/reader.cc | Allows pruned reads of FILE storage to return struct instead of failing extension creation. |
| cpp/src/parquet/arrow/arrow_schema_test.cc | Adds schema conversion tests for FILE extension and fallback behavior. |
| cpp/src/parquet/arrow/arrow_reader_writer_test.cc | Adds end-to-end read/write roundtrip + pruning behavior test for FILE. |
| cpp/src/generated/parquet_types.tcc | Regenerated thrift (adds FileType, Int96TimestampOrder, etc.). |
| cpp/src/generated/parquet_types.h | Regenerated thrift headers (new structs/enums and union members). |
| cpp/src/generated/parquet_types.cpp | Regenerated thrift sources (new struct implementations, enum maps). |
| cpp/src/arrow/meson.build | Adds extension/parquet_file.cc to Arrow C++ build. |
| cpp/src/arrow/extension/parquet_file.h | New FileExtensionType public header and extension name constant. |
| cpp/src/arrow/extension/parquet_file.cc | Implements FileExtensionType validation/serialization/array construction. |
| cpp/src/arrow/extension/parquet_file_test.cc | Adds validation tests for unsupported storage schemas. |
| cpp/src/arrow/extension/meson.build | Adds Meson test target and installs new header. |
| cpp/src/arrow/extension/CMakeLists.txt | Adds CMake test target for the new extension tests. |
| cpp/src/arrow/CMakeLists.txt | Adds extension/parquet_file.cc to Arrow library sources. |
Review details
Suppressed comments (1)
cpp/src/parquet/arrow/schema.cc:1203
FileStorageTypesCompatibleassumes both inputs are struct types and useschecked_cast<const StructType&>(*inferred_type)without checkingid(). In release buildschecked_castis astatic_cast, so a non-struct type here is UB. Also, compatibility currently checks only field names/count, not the corresponding field types/nullability, which can lead to attempting to restore the FILE extension onto an incompatible inferred storage type.
bool FileStorageTypesCompatible(const std::shared_ptr<::arrow::DataType>& origin_type,
const std::shared_ptr<::arrow::DataType>& inferred_type) {
if (origin_type->num_fields() != inferred_type->num_fields()) {
return false;
}
const auto& inferred_struct_type =
checked_cast<const ::arrow::StructType&>(*inferred_type);
for (const auto& origin_field : origin_type->fields()) {
if (inferred_struct_type.GetFieldByName(origin_field->name()) == nullptr) {
- Files reviewed: 15/18 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (extension_type) { | ||
| ARROW_ASSIGN_OR_RAISE( | ||
| struct_type, | ||
| extension_type->Deserialize(std::move(struct_type), /*serialized_data=*/"")); |
There was a problem hiding this comment.
Here, any FILE-annotated group whose fields don't match the recognized set makes the entire read fail (Status::Invalid) rather than falling back to a plain struct.
Should we consider attempting Deserialize and keeping the plain struct_type on error? This would mirror variant's current behavior.
There was a problem hiding this comment.
Sorry, I didn't quite catch what you meant. As I understand it, variant would return Status::Invalid in VariantExtensionType::Make, causing an error here as well.
My reasoning here is that if a type is FILE-annotated and was written by us, we guarantee its correctness. If it was written by another writer, then it should be attributed to that writer's implementation. If we were to allow arbitrary types to pass through, other code in the repository might encounter unknown errors when processing this data.
77011b6 to
b3146d6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
There is an unsafe downcast in the Arrow→Parquet schema conversion for the file extension that can lead to undefined behavior if a differently-implemented extension shares the same extension name.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 15/18 changed files
- Comments generated: 1
- Review effort level: Lite
b3146d6 to
abb5bae
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two critical schema.cc issues remain unresolved: FILE child matching and unsafe inferred-type casting.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 2
- Review effort level: Lite
Rationale for this change
Implement apache/parquet-format#585
Since Arrow lacks specifications related to the
FILEtype, an Arrow extension type was defined to enable read-write round-trips for it. We can wait #51143 to be implemented or we can split this PR into two.What changes are included in this PR?
Implement the Parquet
FILElogical type.Are these changes tested?
Yes.
Are there any user-facing changes?
cpp/src/parquet/types.h.cpp/src/arrow/extension/parquet_file.hand we can replace it at any time.