funcheetah commented on code in PR #4242:
URL: https://github.com/apache/iceberg/pull/4242#discussion_r860277867
##########
core/src/main/java/org/apache/iceberg/avro/AvroSchemaWithTypeVisitor.java:
##########
@@ -79,11 +79,25 @@
private static <T> T visitUnion(Type type, Schema union,
AvroSchemaWithTypeVisitor<T> visitor) {
List<Schema> types = union.getTypes();
List<T> options = Lists.newArrayListWithExpectedSize(types.size());
- for (Schema branch : types) {
- if (branch.getType() == Schema.Type.NULL) {
- options.add(visit((Type) null, branch, visitor));
- } else {
- options.add(visit(type, branch, visitor));
+
+ // simple union case
+ if (AvroSchemaUtil.isOptionSchema(union)) {
+ for (Schema branch : types) {
+ if (branch.getType() == Schema.Type.NULL) {
+ options.add(visit((Type) null, branch, visitor));
+ } else {
+ options.add(visit(type, branch, visitor));
+ }
+ }
+ } else { // complex union case
+ int index = 1;
+ for (Schema branch : types) {
+ if (branch.getType() == Schema.Type.NULL) {
+ options.add(visit((Type) null, branch, visitor));
+ } else {
+ options.add(visit(type.asStructType().fields().get(index).type(),
branch, visitor));
Review Comment:
Discussed with @wmoustafa and @shardulm94 . We agreed that it is okay to
implement Avro complex union logic in `AvroSchemaWithTypeVisitor`. Added
validation to ensure that when visiting Avro complex union, Iceberg type should
be `Types.StructType`.
##########
core/src/main/java/org/apache/iceberg/avro/AvroSchemaVisitor.java:
##########
@@ -52,8 +52,21 @@
case UNION:
List<Schema> types = schema.getTypes();
List<T> options = Lists.newArrayListWithExpectedSize(types.size());
- for (Schema type : types) {
- options.add(visit(type, visitor));
+ if (AvroSchemaUtil.isOptionSchema(schema)) {
+ for (Schema type : types) {
+ options.add(visit(type, visitor));
+ }
+ } else {
+ // complex union case
Review Comment:
Discussed with @wmoustafa and @shardulm94 . We agreed that it is okay to
implement Avro complex union logic in `AvroSchemaVisitor`
##########
core/src/main/java/org/apache/iceberg/avro/AvroSchemaUtil.java:
##########
@@ -154,6 +154,27 @@ public static boolean isOptionSchema(Schema schema) {
return false;
}
+ /**
+ * This method decides whether a schema is of type union and is complex
union and is optional
+ *
+ * Complex union: the number of options in union not equals to 2
+ * Optional: null is present in union
+ *
+ * @param schema input schema
+ * @return true if schema is complex union and it is optional
+ */
+ public static boolean isOptionalComplexUnion(Schema schema) {
+ if (schema.getType() == UNION && schema.getTypes().size() != 2) {
Review Comment:
Updated the check to `schema.getTypes().size() > 2` as suggested.
--
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]
