From b276d3fdcf03a5933a2d21f6ee67394c63584cac Mon Sep 17 00:00:00 2001 From: Aihua Xu Date: Fri, 20 Dec 2024 20:17:35 -0800 Subject: [PATCH] Address comments --- .../main/java/org/apache/iceberg/types/Types.java | 8 ++++++++ .../org/apache/iceberg/types/TestTypeUtil.java | 8 ++++---- .../main/java/org/apache/iceberg/SchemaParser.java | 7 +------ .../iceberg/avro/TestAvroSchemaProjection.java | 14 -------------- .../apache/iceberg/avro/TestSchemaConversions.java | 13 +++++++++++++ 5 files changed, 26 insertions(+), 24 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 index ec1904b10f09..68d6680db8e3 100644 --- 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 index e6740d8efc8b..8d379f1ad0f7 100644 --- 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,17 @@ 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(15, "B", testType))); Schema actualSchema = TypeUtil.reassignOrRefreshIds(schema, sourceSchema); Schema expectedSchema = @@ -630,7 +630,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 index 346aabc120ae..22ad0e9af708 100644 --- 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 index cff99443db1b..b71280daf8db 100644 --- 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 index e135364bca66..d279ab6a4baf 100644 --- 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(); + } }