fix(arrow/extensions): use canonical Variant extension name - #1241
fix(arrow/extensions): use canonical Variant extension name#1241dgvj-work wants to merge 1 commit into
Conversation
Arrow requires arrow.parquet.variant. Keep parquet.variant registered so older IPC still deserializes. Fixes apache#1203 Signed-off-by: Digvijay <digvijay.vaghela@yahoo.com>
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
The legacy Variant compatibility path works when reading old IPC data, but ToParquet can now panic for the newly registered legacy extension type.
The name-based predicate accepts parquet.variant, after which the code asserts that the value is the canonical *extensions.VariantType. A schema containing the registry-returned legacy type therefore panics instead of returning an error. Details are inline.
Targeted extension, IPC, pqarrow, race, and vet tests otherwise pass. This head currently has no CI results.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Arrow Go maintainer. After you've
addressed the points above and pushed an update, an Apache Arrow Go
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Arrow Go handles maintainer review:
CONTRIBUTING.md.
| extType := field.Type.(arrow.ExtensionType) | ||
| if extType.ExtensionName() == "parquet.variant" { | ||
| if extensions.IsVariantExtensionName(extType.ExtensionName()) { | ||
| return variantToNode(extType.(*extensions.VariantType), field, props, arrprops) |
There was a problem hiding this comment.
arrow.GetExtensionType("parquet.variant") now returns *extensions.legacyVariantType. The new name predicate accepts that type, but this unconditional assertion to *extensions.VariantType panics. ToParquet should return an error for unsupported wrappers or normalize compatible legacy storage through NewVariantType, rather than asserting based only on the extension name. Please add a regression test using the registry-returned legacy type.
2 participants
Rationale for this change
The canonical Variant extension type name is
arrow.parquet.variant. arrow-go still writesparquet.variant, which predates apache/arrow#47456. That makes Variant IPC unreadable by other Arrow implementations, and vice versa.C++ already made this change in apache/arrow#49082.
Fixes #1203
What changes are included in this PR?
VariantType.ExtensionName()now returnsarrow.parquet.variantparquet.variantregistered so older arrow-go IPC still deserializes asVariantTypeAre these changes tested?
go test ./arrow/extensionsgo test ./parquet/pqarrow -run 'TestConvertSchemaParquetVariant|TestShreddedVariantSchema'Are there any user-facing changes?
Yes — newly written IPC/Flight metadata uses
arrow.parquet.variant. Existingparquet.variantdata still reads.