Address comments

apache · Dec 21, 2024 · fe6038a · fe6038a
1 parent 4bcb86b
commit fe6038a
Show file tree

Hide file tree

Showing 5 changed files with 26 additions and 25 deletions.
diff --git a/api/src/main/java/org/apache/iceberg/types/Types.java b/api/src/main/java/org/apache/iceberg/types/Types.java
@@ -61,6 +61,14 @@ private Types() {}
   private static final Pattern DECIMAL =
       Pattern.compile("decimal\\(\\s*(\\d+)\\s*,\\s*(\\d+)\\s*\\)");
 
+  public static Type typeFromTypeString(String typeString) {
+    if (VariantType.get().toString().equalsIgnoreCase(typeString)) {
+      return Types.VariantType.get();
+    }
+
+    return Types.fromPrimitiveString(typeString);
+  }
+
   public static PrimitiveType fromPrimitiveString(String typeString) {
     String lowerTypeString = typeString.toLowerCase(Locale.ROOT);
     if (TYPES.containsKey(lowerTypeString)) {

diff --git a/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java b/api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java
@@ -607,17 +607,16 @@ public void testReassignOrRefreshIds(Type testType) {
                 required(10, "a", Types.IntegerType.get()),
                 Types.NestedField.required("c")
                     .withId(11)
-                    .ofType(testType)
+                    .ofType(Types.IntegerType.get())
                     .withInitialDefault(23)
                     .withWriteDefault(34)
                     .build(),
-                required(12, "B", Types.IntegerType.get())),
+                required(12, "B", testType)),
             Sets.newHashSet(10));
     Schema sourceSchema =
         new Schema(
             Lists.newArrayList(
-                required(1, "a", Types.IntegerType.get()),
-                required(15, "B", Types.IntegerType.get())));
+                required(1, "a", Types.IntegerType.get()), required(15, "B", testType)));
 
     Schema actualSchema = TypeUtil.reassignOrRefreshIds(schema, sourceSchema);
     Schema expectedSchema =
@@ -630,7 +629,7 @@ public void testReassignOrRefreshIds(Type testType) {
                     .withInitialDefault(23)
                     .withWriteDefault(34)
                     .build(),
-                required(15, "B", Types.IntegerType.get())));
+                required(15, "B", testType)));
 
     assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
   }

diff --git a/core/src/main/java/org/apache/iceberg/SchemaParser.java b/core/src/main/java/org/apache/iceberg/SchemaParser.java
@@ -42,7 +42,6 @@ private SchemaParser() {}
   private static final String STRUCT = "struct";
   private static final String LIST = "list";
   private static final String MAP = "map";
-  private static final String VARIANT = "variant";
   private static final String FIELDS = "fields";
   private static final String ELEMENT = "element";
   private static final String KEY = "key";
@@ -182,11 +181,7 @@ public static String toJson(Schema schema, boolean pretty) {
 
   private static Type typeFromJson(JsonNode json) {
     if (json.isTextual()) {
-      if (VARIANT.equalsIgnoreCase(json.asText())) {
-        return Types.VariantType.get();
-      }
-
-      return Types.fromPrimitiveString(json.asText());
+      return Types.typeFromTypeString(json.asText());
     } else if (json.isObject()) {
       JsonNode typeObj = json.get(TYPE);
       if (typeObj != null) {

diff --git a/core/src/test/java/org/apache/iceberg/avro/TestAvroSchemaProjection.java b/core/src/test/java/org/apache/iceberg/avro/TestAvroSchemaProjection.java
@@ -18,13 +18,11 @@
  */
 package org.apache.iceberg.avro;
 
-import static org.apache.iceberg.types.Types.NestedField.required;
 import static org.assertj.core.api.Assertions.assertThat;
 
 import java.util.Collections;
 import org.apache.avro.SchemaBuilder;
 import org.apache.iceberg.Schema;
-import org.apache.iceberg.types.Types;
 import org.junit.jupiter.api.Test;
 
 public class TestAvroSchemaProjection {
@@ -152,16 +150,4 @@ public void projectWithMapSchemaChanged() {
         .as("Result of buildAvroProjection is missing some IDs")
         .isFalse();
   }
-
-  @Test
-  public void testVariantConversion() {
-    Schema schema = new Schema(required(1, "variantCol", Types.VariantType.get()));
-    org.apache.avro.Schema avroSchema = AvroSchemaUtil.convert(schema.asStruct());
-
-    org.apache.avro.Schema variantSchema = avroSchema.getField("variantCol").schema();
-    assertThat(variantSchema.getType()).isEqualTo(org.apache.avro.Schema.Type.RECORD);
-    assertThat(variantSchema.getFields().size()).isEqualTo(2);
-    assertThat(variantSchema.getField("metadata")).isNotNull();
-    assertThat(variantSchema.getField("value")).isNotNull();
-  }
 }
diff --git a/core/src/test/java/org/apache/iceberg/avro/TestSchemaConversions.java b/core/src/test/java/org/apache/iceberg/avro/TestSchemaConversions.java
@@ -368,4 +368,17 @@ public void testFieldDocsArePreserved() {
         Lists.newArrayList(Iterables.transform(origSchema.columns(), Types.NestedField::doc));
     assertThat(fieldDocs).isEqualTo(origFieldDocs);
   }
+
+  @Test
+  public void testVariantConversion() {
+    org.apache.iceberg.Schema schema =
+        new org.apache.iceberg.Schema(required(1, "variantCol", Types.VariantType.get()));
+    org.apache.avro.Schema avroSchema = AvroSchemaUtil.convert(schema.asStruct());
+
+    org.apache.avro.Schema variantSchema = avroSchema.getField("variantCol").schema();
+    assertThat(variantSchema.getType()).isEqualTo(org.apache.avro.Schema.Type.RECORD);
+    assertThat(variantSchema.getFields().size()).isEqualTo(2);
+    assertThat(variantSchema.getField("metadata")).isNotNull();
+    assertThat(variantSchema.getField("value")).isNotNull();
+  }
 }