Skip to content

GH-50891: [C++][Parquet] DON'T MERGE NOW: Support FILE logical type - #50892

Open
HuaHuaY wants to merge 2 commits into
apache:mainfrom
HuaHuaY:implement_file_type
Open

GH-50891: [C++][Parquet] DON'T MERGE NOW: Support FILE logical type#50892
HuaHuaY wants to merge 2 commits into
apache:mainfrom
HuaHuaY:implement_file_type

Conversation

@HuaHuaY

@HuaHuaYHuaHuaY commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Implement apache/parquet-format#585

The Parquet format supporting the FILE type has not yet been released, so the corresponding functionality cannot currently be merged into the Arrow repository.

Since Arrow lacks specifications related to the FILE type, an Arrow extension type was defined to enable read-write round-trips for it.

What changes are included in this PR?

  1. update parquet.thrift and the generated code
  2. implement the Parquet FILE logical type

I will split this PR into two after the new Parquet release is published.

Are these changes tested?

Yes.

Are there any user-facing changes?

  1. Add a new logical type at cpp/src/parquet/types.h.
  2. Add a temporary extension type at cpp/src/arrow/extension/parquet_file.h and we can replace it at any time.

@github-actions

Copy link
Copy Markdown

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?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

See also:

@github-actions
github-actionsBot marked this pull request as draft August 17, 2026 07:49
@github-actionsgithub-actionsBot added the awaiting review Awaiting review label Aug 17, 2026
@HuaHuaYHuaHuaY changed the title DON'T MERGE NOW: GH-50891: [C++][Parquet] Support FILE logical typeGH-50891: [C++][Parquet] DON'T MERGE NOW: Support FILE logical typeAug 17, 2026
@HuaHuaY
HuaHuaY marked this pull request as ready for review August 17, 2026 07:49
@github-actions

Copy link
Copy Markdown

⚠️ GitHub issue #50891has been automatically assigned in GitHub to PR creator.

@HuaHuaY

Copy link
Copy Markdown
ContributorAuthor

cc @brkyvz

CopilotAI lite review requested due to automatic review settings September 2, 2026 03:00

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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 FILE as a Parquet logical type (LogicalType::File() / FileLogicalType) with thrift serialization support.
  • Implement Arrow<->Parquet schema conversion for FILE via a new Arrow extension type and ensure column-pruning behavior returns a struct when only part of the storage is read.
  • Update parquet.thrift and regenerated thrift outputs; add unit tests covering schema conversion and read/write round-trips.
File summaries
FileDescription
cpp/src/parquet/types.hAdds FILE logical type API and FileLogicalType declaration.
cpp/src/parquet/types.ccImplements FILE logical type behavior, thrift conversion, and nesting classification.
cpp/src/parquet/schema_test.ccExtends logical type tests for FILE creation/properties/roundtrip.
cpp/src/parquet/parquet.thriftAdds FileType to LogicalType union (and other upstream thrift updates).
cpp/src/parquet/arrow/schema.ccAdds Arrow extension mapping to/from Parquet FILE; metadata restoration tweaks.
cpp/src/parquet/arrow/reader.ccAllows pruned reads of FILE storage to return struct instead of failing extension creation.
cpp/src/parquet/arrow/arrow_schema_test.ccAdds schema conversion tests for FILE extension and fallback behavior.
cpp/src/parquet/arrow/arrow_reader_writer_test.ccAdds end-to-end read/write roundtrip + pruning behavior test for FILE.
cpp/src/generated/parquet_types.tccRegenerated thrift (adds FileType, Int96TimestampOrder, etc.).
cpp/src/generated/parquet_types.hRegenerated thrift headers (new structs/enums and union members).
cpp/src/generated/parquet_types.cppRegenerated thrift sources (new struct implementations, enum maps).
cpp/src/arrow/meson.buildAdds extension/parquet_file.cc to Arrow C++ build.
cpp/src/arrow/extension/parquet_file.hNew FileExtensionType public header and extension name constant.
cpp/src/arrow/extension/parquet_file.ccImplements FileExtensionType validation/serialization/array construction.
cpp/src/arrow/extension/parquet_file_test.ccAdds validation tests for unsupported storage schemas.
cpp/src/arrow/extension/meson.buildAdds Meson test target and installs new header.
cpp/src/arrow/extension/CMakeLists.txtAdds CMake test target for the new extension tests.
cpp/src/arrow/CMakeLists.txtAdds extension/parquet_file.cc to Arrow library sources.
Review details

Suppressed comments (1)

cpp/src/parquet/arrow/schema.cc:1203

  • FileStorageTypesCompatible assumes both inputs are struct types and uses checked_cast<const StructType&>(*inferred_type) without checking id(). In release builds checked_cast is a static_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.

Comment threadcpp/src/parquet/arrow/schema.cc
@github-actionsgithub-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 2, 2026

@dtenedordtenedor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for working on this!

if (extension_type) {
ARROW_ASSIGN_OR_RAISE(
struct_type,
extension_type->Deserialize(std::move(struct_type), /*serialized_data=*/""));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@HuaHuaYHuaHuaYSep 3, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

for (int i = 0; i < inferred_type->num_fields(); ++i) {
std::shared_ptr<::arrow::Field> origin_child;
if (match_children_by_name) {
origin_child = checked_cast<const ::arrow::StructType&>(*origin_type)

@dtenedordtenedorSep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

GetFieldByName returns nullptr if the name is missing or ambiguous. Should we DCHECK(origin_child != nullptr) (or return Status::Invalid)?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I am fine to add DCHECK(origin_child != nullptr).

Comment on lines +1077 to +1079
auto storage = ::arrow::struct_({::arrow::field("uri", ::arrow::utf8()),
::arrow::field("offset", ::arrow::int64()),
::arrow::field("inline", ::arrow::binary())});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The accepted storage types are broader than what's tested: IsSupportedField allows large_utf8/utf8_view and large_binary/binary_view, but round-trip tests only use utf8/int64/binary. Should we add at least one view/large variant round-trip?


namespace arrow::extension {

TEST(FileType, InvalidStorage) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Other test ideas:

  • an unknown extra field alongside the valid ones
  • a REQUIRED field among otherwise-valid ones
  • wrong types for content_type/checksum/size (only uri/offset/inline type-errors are tested)

/** Adaptive Lossless floating-Point (ALP) encoding for FLOAT and DOUBLE.
Losslessly converts decimal-like floating-point values to integers via
decimal scaling, then applies Frame of Reference (FOR) encoding and
bit-packing; values that cannot be converted losslessly are stored as

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is it intended to bring this in as well in this PR?

@HuaHuaYHuaHuaYSep 3, 2026

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No. Since the Parquet version containing the FILE type has not yet been released, I don't know whether it's appropriate to update parquet.thrift in the Arrow repository prematurely. So this PR currently incorporates the contents of two separate PRs.

  1. update parquet.thrift and the generated code
  2. implement the Parquet FILE logical type

Code related to ALP is included in the first part. I will split this PR into two after the new Parquet release is published.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@HuaHuaY@dtenedor