aihuaxu commented on code in PR #13941:
URL: https://github.com/apache/iceberg/pull/13941#discussion_r2317665752
##########
api/src/main/java/org/apache/iceberg/variants/Variant.java:
##########
@@ -22,6 +22,10 @@
/** A variant metadata and value pair. */
public interface Variant {
Review Comment:
In TypeWithShemaVisitor.java, there is another place that we need to change.
Can you also update there?
```
} else if (annotation instanceof
LogicalTypeAnnotation.VariantLogicalTypeAnnotation || (iType != null &&
iType.isVariantType())) {
// when Parquet has a VARIANT logical type, use it here
return visitVariant(iType.asVariantType(), group, visitor);
}
```
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/data/ParquetWithSparkSchemaVisitor.java:
##########
@@ -154,11 +155,10 @@ public static <T> T visit(DataType sType, Type type,
ParquetWithSparkSchemaVisit
} finally {
visitor.fieldNames.pop();
}
- } else if (sType instanceof VariantType) {
- // TODO: Use LogicalTypeAnnotation.variantType().equals(annotation)
when VARIANT type is
- // added to Parquet
- // Preconditions.checkArgument(
- // sType instanceof VariantType, "Invalid variant: %s is not a
VariantType", sType);
+ } else if (sType instanceof VariantType
Review Comment:
Agree that we need to handle both cases (missing annotation but sType is
variant and the logical type is variant) to support the existing data.
Also should we switch to fallback the old way as
`LogicalTypeAnnotation.variantType(Variant.VARIANT_SPEC_VERSION).equals(annotation)
|| sType instanceof VariantType`.
--
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]
