Skip to content

feat(arrow): recognize arrow.parquet.variant on schema import#2840

Merged
CTTY merged 1 commit into
apache:mainfrom
c-thiel:feat/arrow-to-iceberg-variant
Jul 20, 2026
Merged

feat(arrow): recognize arrow.parquet.variant on schema import#2840
CTTY merged 1 commit into
apache:mainfrom
c-thiel:feat/arrow-to-iceberg-variant

Conversation

@c-thiel

@c-thiel c-thiel commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Teach arrow_schema_to_schema to fold a field tagged with the canonical arrow.parquet.variant extension into an atomic Type::Variant, the inverse of the export path added in #2188. Previously such a schema errored with "Field id not found in metadata" because the variant's metadata/value storage sub-fields carry no field id.

Which issue does this PR close?

PR 1 of 2 for Variant read support.

What changes are included in this PR?

Add a defaulted ArrowSchemaVisitor::variant dispatched via a new visit_field helper at each field-dispatch site. The default re-enters normal traversal, so the reconstructing visitors (Int96Coercion, MetadataStrip) are unchanged; ArrowSchemaConverter overrides it to fold the struct without descending, guarding that the extension sits on Struct storage. Mirrors Java's Parquet reader, which recovers a variant from its group-level annotation without visiting the group's children.

Covers top-level, struct-nested, list-element and map-value positions plus auto-assigned ids; regenerates the iceberg public-api baseline.

Are these changes tested?

unit-tested yes.

Teach arrow_schema_to_schema to fold a field tagged with the canonical
arrow.parquet.variant extension into an atomic Type::Variant, the inverse
of the export path added in apache#2188. Previously such a schema errored with
"Field id not found in metadata" because the variant's metadata/value
storage sub-fields carry no field id.

@CTTY CTTY left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM! Just one comment

fn variant(&mut self, field: &FieldRef) -> Result<Self::T> {
// The extension may only sit on struct storage (mirrors
// `VariantExtensionType::supports_data_type`).
if !matches!(field.data_type(), DataType::Struct(_)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we match the nested types here as well (metadata and value)?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I thought about this too - my main reasons for not matching the nested types are:

  1. I believe when we use shredding the storage has typed value fields, not just {metadata, value}
  2. It mirrors Java (which isn't necessarily a good reason though): MessageTypeToType.variant() returns VariantType.get() and ParquetTypeVisitor short-circuits the group without validating its children; structural checks are deferred to read time.
  3. Recognition's job is "is this a Variant?" — the extension answers that. Validating we can actually read the storage (and rejecting shredded for now) belongs in the read PR

/// single logical variant.
///
/// Takes the `&FieldRef` rather than a `&DataType` because the variant signal
/// lives on the field's metadata, not on its data type.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Makes sense

@CTTY CTTY left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR!

@CTTY
CTTY merged commit 7c1e0fa into apache:main Jul 20, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants