zeroshade commented on code in PR #932:
URL: https://github.com/apache/iceberg-go/pull/932#discussion_r3126456838
##########
table/internal/parquet_files.go:
##########
@@ -201,6 +201,8 @@ func (parquetFormat) PrimitiveTypeToPhysicalType(typ
iceberg.PrimitiveType) stri
return "BYTE_ARRAY"
case iceberg.DecimalType:
return "FIXED_LEN_BYTE_ARRAY"
+ case iceberg.VariantType:
Review Comment:
By the spec, I `Variant` is not considered a "Primitive" type, so this case
should never happen (and we should make sure that Variant is not a
`PrimitiveType`
##########
table/internal/parquet_files.go:
##########
@@ -1075,6 +1080,14 @@ func (p *pruneParquetSchema) Field(field
pqarrow.SchemaField, result arrow.Field
return result
}
+ // Variant is an extension type wrapping a struct (metadata + value).
+ // Select the entire field including all children.
+ if ext, ok := field.Field.Type.(arrow.ExtensionType); ok &&
ext.ExtensionName() == "parquet.variant" {
+ p.collectLeafIndices(field)
Review Comment:
should we just have a `projectVariant`?
##########
table/substrait/substrait.go:
##########
@@ -170,6 +170,11 @@ func (convertToSubstrait) VisitUnknown() types.Type {
return nil
}
+func (convertToSubstrait) VisitVariant() types.Type {
+ // Variant has no Substrait equivalent
+ return nil
+}
Review Comment:
this is fine for now, but we're going to need to potentially create an
extension type for this that we can use to implement handling in arrow-go for
this. We should also have a follow-up PR to properly optimize the scenario
where shredded columns are selected as opposed to the entire variant.
##########
types.go:
##########
@@ -777,6 +779,20 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+// VariantType represents semi-structured data stored using the Parquet Variant
+// binary encoding. Requires format version 3+.
+type VariantType struct{}
+
+func (VariantType) Equals(other Type) bool {
+ _, ok := other.(VariantType)
+
+ return ok
+}
+
+func (VariantType) primitive() {}
Review Comment:
as mentioned above, according to the spec, Variant is not a primitive type
##########
table/metadata_schema_compatibility.go:
##########
@@ -168,7 +168,9 @@ func (v *unknownTypeValidator) Field(field
iceberg.NestedField, fieldResult erro
if fieldResult != nil {
return fieldResult
}
- if _, isUnknown := field.Type.(iceberg.UnknownType); isUnknown {
+ _, isUnknown := field.Type.(iceberg.UnknownType)
+ _, isVariant := field.Type.(iceberg.VariantType)
+ if isUnknown || isVariant {
Review Comment:
variant can absolutely be required as far as i'm aware.
##########
exprs.go:
##########
@@ -638,6 +640,8 @@ func createBoundUnaryPredicate(op Operation, term
BoundTerm) BoundUnaryPredicate
return newBoundUnaryPred[Decimal](op, term)
case UUIDType:
return newBoundUnaryPred[uuid.UUID](op, term)
+ case VariantType:
+ return newBoundUnaryPred[[]byte](op, term)
Review Comment:
same comment as above
##########
exprs.go:
##########
@@ -450,6 +450,8 @@ func createBoundRef(field NestedField, acc accessor)
BoundReference {
return &boundRef[Decimal]{field: field, acc: acc}
case UUIDType:
return &boundRef[uuid.UUID]{field: field, acc: acc}
+ case VariantType:
+ return &boundRef[[]byte]{field: field, acc: acc}
Review Comment:
I wouldn't use the `[]byte` here at all and would instead properly use
`variant.Value` and add the appropriate case. Particularly because
`variant.Value` is a struct and we don't want to set a precedent for coercing
it to a `[]byte`. Notice how we properly use `uuid.UUID` rather than a `[]byte`
or `[16]byte`.
##########
table/internal/parquet_files.go:
##########
@@ -573,7 +575,10 @@ func (p parquetFormat) DataFileStatsFromMeta(meta
Metadata, statsCols map[int]St
panic(err)
}
- fieldID := colMapping[colChunk.PathInSchema().String()]
+ fieldID, ok :=
colMapping[colChunk.PathInSchema().String()]
+ if !ok {
Review Comment:
agreed that we need to narrow this to skip ONLY the variant sub-columns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
