aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1947067468
##########
core/src/test/java/org/apache/iceberg/avro/TestSchemaConversions.java:
##########
@@ -370,4 +370,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();
Review Comment:
For test cases like `testPrimitiveTypes`, right now we have the same
dependency (as BuildAvroProjection) on adding Variant logical type to Avro. I
will add that later.
##########
data/src/test/java/org/apache/iceberg/data/avro/TestGenericData.java:
##########
@@ -76,4 +86,29 @@ protected void writeAndValidate(Schema writeSchema, Schema
expectedSchema) throw
protected boolean supportsDefaultValues() {
return true;
}
+
+ @Test
+ public void testSchemaWithTwoVariants() throws JsonProcessingException {
Review Comment:
I was trying to have test coverage for two variant columns in one Avro
schema. Since it can be covered with testVariantConversion(), I can remove this
one.
##########
core/src/test/java/org/apache/iceberg/avro/TestSchemaConversions.java:
##########
@@ -370,4 +370,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();
Review Comment:
I changed the test to test with 2 variant columns now. I think we don't need
to keep the test for 1 variant column so I don't keep it here.
##########
core/src/test/java/org/apache/iceberg/avro/TestBuildAvroProjection.java:
##########
@@ -401,4 +402,31 @@ public void projectMapWithLessFieldInValueSchema() {
.as("Unexpected value ID discovered on the projected map schema")
.isEqualTo(1);
}
+
+ @Test
+ public void projectVariantSchemaUnchanged() {
+ final Type icebergType = Types.VariantType.get();
+
+ final org.apache.avro.Schema expected =
+ SchemaBuilder.record("variant")
+ .prop(AvroSchemaUtil.FIELD_ID_PROP, "1")
+ .namespace("unit.test")
+ .fields()
+ .name("metadata")
+ .type()
+ .bytesType()
+ .noDefault()
+ .name("value")
+ .type()
+ .bytesType()
+ .noDefault()
+ .endRecord();
+
+ final BuildAvroProjection testSubject =
+ new BuildAvroProjection(icebergType, Collections.emptyMap());
+ final org.apache.avro.Schema actual = testSubject.record(expected,
List.of(), null);
+ assertThat(actual)
+ .as("Variant projection produced undesired variant schema")
+ .isEqualTo(expected);
Review Comment:
I will delay the change in BuildAvroProjection as mentioned above so I
removed the test. Will add this back when making the code change.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
