You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aihuaxu (via GitHub)" <gi...@apache.org> on 2024/12/19 22:59:40 UTC
[PR] Core: add variant type support [iceberg]
aihuaxu opened a new pull request, #11831:
URL: https://github.com/apache/iceberg/pull/11831
This is to add some required changes in API and core module for Variant support, including:
- Add isVariantType() method for variant type
- Add variant support in ReassignIds and TypeUtil
- Add variant support in avro projection.
Fix: #11178
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894351944
##########
api/src/main/java/org/apache/iceberg/transforms/Identity.java:
##########
@@ -39,7 +42,7 @@ class Identity<T> implements Transform<T, T> {
@Deprecated
public static <I> Identity<I> get(Type type) {
Preconditions.checkArgument(
- type.typeId() != Type.TypeID.VARIANT, "Unsupported type for identity: %s", type);
+ !UNSUPPORTED_TYPES.contains(type), "Unsupported type for identity: %s", type);
Review Comment:
I probably wouldn't do this refactoring until there's some other type where identity partition isn't supported, but just a nit.
##########
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();
+ 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();
Review Comment:
Should we have stronger assertions on the types of these fields (they should be bytes)?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894554408
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -42,6 +42,7 @@ 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";
Review Comment:
And also fromPrimitiveString() is returning PrimitiveType. I think it's better not to mix variant with primitive. I create typeFromTypeString(String typeString) to handle both cases. Let me know how that looks.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894518142
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -709,6 +709,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
Would it be safer to throw `UnsupportedOperationException` here instead? I think that's what we have done for expression visitors when we add new cases.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894519134
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroSchemaProjection.java:
##########
@@ -150,4 +152,16 @@ public void projectWithMapSchemaChanged() {
.as("Result of buildAvroProjection is missing some IDs")
.isFalse();
}
+
+ @Test
+ public void testVariantConversion() {
Review Comment:
Is the projection test suite the right place for conversion tests? Isn't there a conversion test suite?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894518504
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -166,6 +169,10 @@ public static String toJson(Schema schema, boolean pretty) {
private static Type typeFromJson(JsonNode json) {
if (json.isTextual()) {
+ if (VARIANT.equalsIgnoreCase(json.asText())) {
Review Comment:
I think `fromPrimitiveString` should handle this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927933385
##########
core/src/main/java/org/apache/iceberg/avro/BuildAvroProjection.java:
##########
@@ -56,6 +56,10 @@ class BuildAvroProjection extends AvroCustomOrderSchemaVisitor<Schema, Schema.Fi
@Override
@SuppressWarnings("checkstyle:CyclomaticComplexity")
public Schema record(Schema record, List<String> names, Iterable<Schema.Field> schemaIterable) {
+ if (current.isVariantType()) {
Review Comment:
I think that the corresponding `visit` method should be updated to call a new visitor `variant` method. That will be cleaner.
The visitor should look for the `variant` logical type, so we will need to implement the logical type.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927935593
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -52,6 +56,15 @@ public class TestMetadataUpdateParser {
Types.NestedField.required(1, "id", Types.IntegerType.get()),
Types.NestedField.optional(2, "data", Types.StringType.get()));
+ private static final Schema ID_VARIANTDATA_SCHEMA =
Review Comment:
There is a `TestSchemaParser` class now.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1946870340
##########
core/src/main/java/org/apache/iceberg/avro/BuildAvroProjection.java:
##########
@@ -56,6 +56,10 @@ class BuildAvroProjection extends AvroCustomOrderSchemaVisitor<Schema, Schema.Fi
@Override
@SuppressWarnings("checkstyle:CyclomaticComplexity")
public Schema record(Schema record, List<String> names, Iterable<Schema.Field> schemaIterable) {
+ if (current.isVariantType()) {
Review Comment:
You mean handle variant in the following visit method? As I mentioned that we don't have variant logical type in Schema.getType() from avro, we can't handle that for now. Let me know if understand correctly. I think this is needed for metadata change and I can defer until then.
> abstract class AvroCustomOrderSchemaVisitor<T, F> {
> public static <T, F> T visit(Schema schema, AvroCustomOrderSchemaVisitor<T, F> visitor) {
> switch (schema.getType()) {
>
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894549981
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -132,6 +133,8 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws
static void toJson(Type type, JsonGenerator generator) throws IOException {
if (type.isPrimitiveType()) {
toJson(type.asPrimitiveType(), generator);
+ } else if (type.isVariantType()) {
+ generator.writeString(type.toString());
Review Comment:
Currently we don't have `TestSchemaParser`. I'm trying to use TestMetadataUpdateParser.java indirectly test the change here. Let me know if we should create `TestSchemaParser`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927936907
##########
core/src/test/java/org/apache/iceberg/avro/TestBuildAvroProjection.java:
##########
@@ -401,4 +402,32 @@ 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")
+ .namespace("unit.test")
+ .fields()
+ .name("metadata")
Review Comment:
I don't think that variant `value` and `metadata` columns are assigned IDs. Instead, the variant field is assigned an ID and these are left alone.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1928072103
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -709,6 +709,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
Sounds good. I think I may have relied on the null value from this and not updating in some existing implementation. Let me update that.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1937744616
##########
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) {
Review Comment:
@amogh-jahagirdar Do you prefer to add a new `fromTypeString` or change `fromPrimitiveString` to `fromTypeString` (that introduces incompatibility in the api)? Would it be fine to introduce incompatibility - assume that will be in 1.8?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1900477885
##########
api/src/main/java/org/apache/iceberg/transforms/Identity.java:
##########
@@ -39,7 +42,7 @@ class Identity<T> implements Transform<T, T> {
@Deprecated
public static <I> Identity<I> get(Type type) {
Preconditions.checkArgument(
- type.typeId() != Type.TypeID.VARIANT, "Unsupported type for identity: %s", type);
+ !UNSUPPORTED_TYPES.contains(type), "Unsupported type for identity: %s", type);
Review Comment:
Oh, I might have. I would normally structure it as the `contains` at first but wouldn't necessarily make that change to existing code.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942078866
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +647,37 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ @Test
+ public void testVariantType() {
Review Comment:
I think this should be more specific. Right now, it looks like all of the methods that needed to be validated were added here (I see at least `assignFreshIds` and `project`).
Instead, I think that this should locate a test case with each group of tests (for the different `TestUtil` methods) and name it something like `testProjectWithVariantType()` or `testAssignFreshIdsWithVariantType()`.
Another thing that would be nice is to add tests for `NewTypes` instead of just Variant. You could parameterize them and test `TimestampNanoType`, `UnknownType`, as well. (We can also do that in a follow up if you want).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958783694
##########
api/src/main/java/org/apache/iceberg/types/PrimitiveHolder.java:
##########
@@ -33,6 +33,6 @@ class PrimitiveHolder implements Serializable {
}
Object readResolve() throws ObjectStreamException {
- return Types.fromPrimitiveString(typeAsString);
+ return Types.fromTypeName(typeAsString);
Review Comment:
The new type tests have been added in `TestSerializableTypes`. But let me refactor a little bit.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958673483
##########
api/src/main/java/org/apache/iceberg/types/PrimitiveHolder.java:
##########
@@ -33,6 +33,6 @@ class PrimitiveHolder implements Serializable {
}
Object readResolve() throws ObjectStreamException {
- return Types.fromPrimitiveString(typeAsString);
+ return Types.fromTypeName(typeAsString);
Review Comment:
This is tested by `TestSerializableTypes`. Can you add new cases for variant (and the other new types, while you're updating it)?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958687886
##########
api/src/main/java/org/apache/iceberg/types/CheckCompatibility.java:
##########
@@ -250,6 +250,19 @@ public List<String> map(
}
}
+ @Override
+ public List<String> variant(Types.VariantType readVariant) {
+ if (currentType.isVariantType()) {
+ return NO_ERRORS;
+ }
+
+ // Currently promotion is not allowed to variant type
+ return ImmutableList.of(
+ String.format(
+ ": %s cannot be read as a %s",
+ currentType.typeId().toString().toLowerCase(Locale.ENGLISH), readVariant));
Review Comment:
Yeah. You are right. currentType already has toString() implementation.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1936181802
##########
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) {
Review Comment:
I'd be +1 on `fromTypeString`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955149743
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +651,70 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(
+ Arguments.of(Types.UnknownType.get()),
+ Arguments.of(Types.VariantType.get()),
+ Arguments.of(Types.TimestampNanoType.withoutZone()),
+ Arguments.of(Types.TimestampNanoType.withZone()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignFreshIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Schema assignedSchema = TypeUtil.assignFreshIds(schema, new AtomicInteger(10)::incrementAndGet);
+ Schema expectedSchema =
+ new Schema(required(11, "v", testType), required(12, "A", Types.IntegerType.get()));
+ assertThat(assignedSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ Schema reassignedSchema = TypeUtil.reassignIds(schema, sourceSchema);
+ assertThat(reassignedSchema.asStruct()).isEqualTo(sourceSchema.asStruct());
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testIndexByIdWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Map<Integer, Types.NestedField> indexByIds = TypeUtil.indexById(schema.asStruct());
+ assertThat(indexByIds.get(0).type()).isEqualTo(testType);
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testIndexNameByIdWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Map<Integer, String> indexNameByIds = TypeUtil.indexNameById(schema.asStruct());
+ assertThat(indexNameByIds.get(0)).isEqualTo("v");
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testProjectWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Schema expectedSchema = new Schema(Lists.newArrayList(required(0, "v", testType)));
Review Comment:
There's no need for `newArrayList` here. There is a varargs version.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953612580
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -81,6 +82,15 @@ public static PrimitiveType fromPrimitiveString(String typeString) {
throw new IllegalArgumentException("Cannot parse type string to primitive: " + typeString);
}
+ public static PrimitiveType fromPrimitiveString(String typeString) {
+ Type type = fromTypeName(typeString);
+ if (type.isPrimitiveType()) {
+ return (PrimitiveType) type;
Review Comment:
Nit: should use `asPrimitiveType()`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894431630
##########
api/src/main/java/org/apache/iceberg/transforms/Identity.java:
##########
@@ -39,7 +42,7 @@ class Identity<T> implements Transform<T, T> {
@Deprecated
public static <I> Identity<I> get(Type type) {
Preconditions.checkArgument(
- type.typeId() != Type.TypeID.VARIANT, "Unsupported type for identity: %s", type);
+ !UNSUPPORTED_TYPES.contains(type), "Unsupported type for identity: %s", type);
Review Comment:
Why change this? It works just fine right? We can alter this when we add another type.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927919640
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -709,6 +709,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
The `map` and `list` methods have been part of the visitors from the start, so when new visitors are added, the author decides what to do for `map` and `list`. Now that `variant` is added, returning `null` may not be correct.
Consider a simple example:
```java
public static boolean isNestedTypes(Type type) {
return visit(type, new SchemaVisitor<>() {
@Override
public Boolean struct(Types.StructType struct, List<Boolean> fieldResults) {
return true;
}
@Override
public Boolean field(Types.NestedField field, Boolean fieldResult) {
return true;
}
@Override
public Boolean list(Types.ListType list, Boolean elementResult) {
return true;
}
@Override
public Boolean map(Types.MapType map, Boolean keyResult, Boolean valueResult) {
return true;
}
@Override
public Boolean primitive(Type.PrimitiveType primitive) {
return false;
}
});
}
```
When `TypeVisitor` has the new method added, nothing catches that it is not implemented and violates the assumptions of the existing implementation, which is that the methods return a `Boolean`. The result would be a `NullPointerException` when this code encounters a variant. It would be better if the result were a helpful error message that states that the visitor hasn't been updated for variant.
In addition, some visitors use `null` to pass information. For example, `PruneColumns` returns `null` to indicate that a field was not projected. Returning null in those cases could result in incorrect behavior for the visitor. That's why I think it would be safer to throw an `UnsupportedOperationException` when the visitor does not implement `variant`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927925802
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -24,38 +24,43 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.util.Set;
+import java.util.stream.Stream;
import org.apache.iceberg.Schema;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
-import org.apache.iceberg.types.Types.IntegerType;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
public class TestTypeUtil {
- @Test
- public void testReassignIdsDuplicateColumns() {
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(Arguments.of(Types.IntegerType.get()), Arguments.of(Types.VariantType.get()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsDuplicateColumns(Type testType) {
Review Comment:
When updating existing tests, it is better to leave tests alone when they are testing behavior that is not relevant to what you're changing. In this case, the behavior being tested is not related to the type of these columns, it is that the columns duplicate one another.
It is typically a red flag for me to see updates to existing tests like this. When you're adding new behavior, think about what needs to be tested. For variant, this needs to test that variant columns are preserved (not removed, skipped, or replaced with `null`) and are reassigned new IDs. It does not need to test _how_ the IDs are assigned or that ID assignment is case sensitive.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927927230
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -132,6 +133,8 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws
static void toJson(Type type, JsonGenerator generator) throws IOException {
if (type.isPrimitiveType()) {
toJson(type.asPrimitiveType(), generator);
+ } else if (type.isVariantType()) {
+ generator.writeString(type.toString());
Review Comment:
I think I had recently added `TestSchemaParser`. You should be able to update it now.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1940041876
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -166,6 +169,10 @@ public static String toJson(Schema schema, boolean pretty) {
private static Type typeFromJson(JsonNode json) {
if (json.isTextual()) {
+ if (VARIANT.equalsIgnoreCase(json.asText())) {
Review Comment:
From dev list discussion, seems we need to add new one and mark this one deprecated. Then we will deprecate later. I added a new `fromTypeString()`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894546766
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -709,6 +709,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
I can see others like map()/list() all return null here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927930058
##########
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) {
Review Comment:
I also think that if we want to have an additional method to avoid confusion (i.e. "I don't want to `fromPrimitiveString` that because I need support for variant") then we can add a synonym. In that case, I think there's probably a better name, like `fromString`. That's not great because it implies that it would support structs, maps, and lists, but so does `typeFromTypeString` so it's at least more direct. Unfortunately, we don't have a word in Iceberg that means a type that is expressed in a single string (vs a JSON-defined type).
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1929488590
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -108,19 +121,19 @@ public void testUpgradeFormatVersionFromJson() {
}
/** AddSchema * */
- @Test
- public void testAddSchemaFromJson() {
+ @ParameterizedTest
+ @MethodSource("testSchemas")
Review Comment:
I reverted this change and have a new test to cover that.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958636578
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
Added the implementations other than PruneColumnsWithoutReordering. For FixupTypes for Spark/Flink, there is no fixup for Variant and I didn't add coverage for it, unless we just want to test out the exception is not thrown.
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -616,8 +616,8 @@ public T map(Types.MapType map, T keyResult, T valueResult) {
return null;
}
- public T variant() {
- return null;
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
Added `CreateMapping` and `DescribeSchemaVisitor` visitor implementation. I will try to address the remaining from different modules separately.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955196818
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
Looks like this is not yet implemented by:
* `CheckCompatibility` (tested by `TestReadabilityChecks`)
* `FixupTypes`, `SparkFixupTypes`, or `FlinkFixupTypes`
* ~~`PruneColumnsWithoutReordering` (Spark, so maybe this can be done separately)~~
* `ReassignDoc`
* `AssignIds`
It is also not implemented by `PruneColumnsWithReordering` but that doesn't appear to be referenced anywhere. I opened #12258 to remove these.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958675667
##########
core/src/main/java/org/apache/iceberg/mapping/MappingUtil.java:
##########
@@ -302,6 +302,11 @@ public MappedFields map(Types.MapType map, MappedFields keyResult, MappedFields
MappedField.of(map.valueId(), "value", valueResult));
}
+ @Override
+ public MappedFields variant(Types.VariantType variant) {
+ return null; // no mapping because variant no nested fields
Review Comment:
Minor: "no mapping because variant has no nested fields with IDs"
It is missing "has" (typo) and I think it is good to state that there are no nested IDs because there can be nested data within a variant.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942102793
##########
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 think we also need a test that multiple variant columns can be converted in a single Avro record.
Other test cases (like `testPrimitiveTypes`) validate Avro to Iceberg conversion. Do you want to add that as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942093906
##########
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) {
Review Comment:
I commented above, but I think this should be `fromTypeName` to make it more clear that this is not able to construct nested types. I would also keep `fromPrimitiveString` and not deprecate it because it is still useful. We just need to replace all internal uses with `fromTypeName`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942090932
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -145,6 +145,8 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws
static void toJson(Type type, JsonGenerator generator) throws IOException {
if (type.isPrimitiveType()) {
toJson(type.asPrimitiveType(), generator);
+ } else if (type.isVariantType()) {
+ generator.writeString(type.toString());
Review Comment:
I'd probably just inline the `toJson` call from the previous case along with this:
```java
if (type.isPrimitiveType() || type.isVariantType()) {
generator.writeString(type.toString());
} else {
```
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955155159
##########
core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java:
##########
@@ -187,6 +187,18 @@ public Schema map(Types.MapType map, Schema keySchema, Schema valueSchema) {
return mapSchema;
}
+ @Override
+ public Schema variant(Types.VariantType variant) {
Review Comment:
I think this is included (and tested) in #12238. Can you remove it from here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894478633
##########
api/src/main/java/org/apache/iceberg/transforms/Identity.java:
##########
@@ -39,7 +42,7 @@ class Identity<T> implements Transform<T, T> {
@Deprecated
public static <I> Identity<I> get(Type type) {
Preconditions.checkArgument(
- type.typeId() != Type.TypeID.VARIANT, "Unsupported type for identity: %s", type);
+ !UNSUPPORTED_TYPES.contains(type), "Unsupported type for identity: %s", type);
Review Comment:
Yes. It works fine. I thought you mentioned that you wanted to make such change in a previous PR. :)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942088807
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -81,6 +82,16 @@ public static PrimitiveType fromPrimitiveString(String typeString) {
throw new IllegalArgumentException("Cannot parse type string to primitive: " + typeString);
}
+ @Deprecated
Review Comment:
Since this has a different return type, it makes sense to me that it would be supported. I think the motivation to deprecate it is that we want uses to move to `fromString` / `fromTypeName`, but we can do that by searching for uses and updating.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942080873
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -709,6 +709,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
Yeah, this should help us identify what needs to be tested as well!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953616108
##########
api/src/test/java/org/apache/iceberg/types/TestTypes.java:
##########
@@ -43,6 +67,13 @@ public void fromPrimitiveString() {
assertThat(Types.fromPrimitiveString("Decimal(2,3)")).isEqualTo(Types.DecimalType.of(2, 3));
+ assertThatExceptionOfType(IllegalArgumentException.class)
+ .isThrownBy(() -> Types.fromPrimitiveString("variant"))
+ .withMessage("Cannot parse type string to primitive: variant");
Review Comment:
I think this message could be confusing. When this happens, we know that the type name is valid. The problem is that it can't be returned as a `PrimitiveType`. I would prefer to say that in the error: `"Cannot parse type string: variant is not a primitive type"`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953676388
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
Yeah. I missed that. Multiple of them. I also updated to pass the type here so we will return the same instance.
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
Yeah. I missed that. I also updated to pass the type here so we will return the same instance.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1960094606
##########
api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java:
##########
@@ -46,6 +46,7 @@ public void testIdentityTypes() throws Exception {
Types.StringType.get(),
Types.UUIDType.get(),
Types.BinaryType.get(),
+ Types.UnknownType.get()
Review Comment:
@aihuaxu, why not add variant as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958783694
##########
api/src/main/java/org/apache/iceberg/types/PrimitiveHolder.java:
##########
@@ -33,6 +33,6 @@ class PrimitiveHolder implements Serializable {
}
Object readResolve() throws ObjectStreamException {
- return Types.fromPrimitiveString(typeAsString);
+ return Types.fromTypeName(typeAsString);
Review Comment:
The new type tests have been added in `TestSerializableTypes`. But let me refactor a little bit for testUnknown() which should be tested with `isSameAs()`, same as other primitive types. `testVariant()` is testing with `isEqualTo()`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927930058
##########
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) {
Review Comment:
I also think that if we want to have an additional method to avoid confusion (i.e. "I don't want to `fromPrimitiveString` that because I need support for variant") then we can add a synonym. In that case, I think there's probably a better name, like `fromString`. That's not great because it implies that it would support structs, maps, and lists, but so does `typeFromTypeString` so it's at least more direct. Unfortunately, we don't have a word in Iceberg that means a type that is expressed in a single string (vs a JSON-defined type).
I would probably just leave this as `fromPrimitiveString` and not worry about it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927935200
##########
core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java:
##########
@@ -49,6 +49,15 @@ abstract class TypeToSchema extends TypeUtil.SchemaVisitor<Schema> {
private static final Schema UUID_SCHEMA =
LogicalTypes.uuid().addToSchema(Schema.createFixed("uuid_fixed", null, null, 16));
private static final Schema BINARY_SCHEMA = Schema.create(Schema.Type.BYTES);
+ private static final Schema VARIANT_SCHEMA =
Review Comment:
I think this will break when there are multiple variant columns because Avro requires each record to have a unique name. This is why record names are generated from field IDs. Can you create a test case to validate this?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1904474514
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -52,6 +56,15 @@ public class TestMetadataUpdateParser {
Types.NestedField.required(1, "id", Types.IntegerType.get()),
Types.NestedField.optional(2, "data", Types.StringType.get()));
+ private static final Schema ID_VARIANTDATA_SCHEMA =
Review Comment:
Let me know if we need to add TestSchemaParser class. Not sure if we already cover the tests with TestMetadataUpdateParser.java indirectly and intentionally not add TestSchemaParser.java.
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -42,6 +42,7 @@ 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";
Review Comment:
@rdblue Can you help check if the new change works for you?
##########
api/src/main/java/org/apache/iceberg/transforms/Identity.java:
##########
@@ -39,7 +42,7 @@ class Identity<T> implements Transform<T, T> {
@Deprecated
public static <I> Identity<I> get(Type type) {
Preconditions.checkArgument(
- type.typeId() != Type.TypeID.VARIANT, "Unsupported type for identity: %s", type);
+ !UNSUPPORTED_TYPES.contains(type), "Unsupported type for identity: %s", type);
Review Comment:
I don't follow the following change suggestion. Let me revert this unrelated change and we can add it back if needed.
> I would normally structure it as the contains at first
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955156020
##########
core/src/main/java/org/apache/iceberg/schema/SchemaWithPartnerVisitor.java:
##########
@@ -106,7 +106,8 @@ public static <P, T> T visit(
}
return visitor.map(map, partner, keyResult, valueResult);
-
+ case VARIANT:
+ return visitor.variant(type.asVariantType(), partner);
Review Comment:
Nit: other branches have an empty newline after between cases.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958670627
##########
api/src/test/java/org/apache/iceberg/types/TestReadabilityChecks.java:
##########
@@ -112,6 +114,34 @@ private void testDisallowPrimitiveToStruct(PrimitiveType from, Schema fromSchema
.contains("cannot be read as a struct");
}
+ @Test
+ public void testVariantType() {
+ Schema fromSchema = new Schema(required(1, "from_field", Types.VariantType.get()));
+ List<String> errors =
+ CheckCompatibility.writeCompatibilityErrors(
+ new Schema(required(1, "to_field", Types.VariantType.get())), fromSchema);
+ assertThat(errors).as("Should produce 0 error messages").isEmpty();
Review Comment:
This combines two different test cases that I think should be separate. The first is this check that variant is compatible with itself. The second is the case of all other types being written to or read as variant.
There are two problems with combining separate test cases in one method.
* For one, things that are tested first can hide other issues that are tested later. That can lead to frustrating development were you keep squashing test bugs, but there are more (different) errors coming from the same method. It's better to split these so that all errors are visible at the same time. That way when tests are failing you can tackle the simplest cases first and usually fix problems before getting to the complicated cases. (A good example of this is the set of cases in `DataTest`, where tests get more complex: `testArray`, `testArrayOfStructs`, `testMap`, `testNumericMapKey`, ... `testMixedTypes`.)
* The second issue is that packing cases into one method can't take advantage of newer test framework features, like `@ParameterizedTest` in JUnit5 that would work well for the incompatible types here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955154197
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +651,70 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(
+ Arguments.of(Types.UnknownType.get()),
+ Arguments.of(Types.VariantType.get()),
+ Arguments.of(Types.TimestampNanoType.withoutZone()),
+ Arguments.of(Types.TimestampNanoType.withZone()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignFreshIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Schema assignedSchema = TypeUtil.assignFreshIds(schema, new AtomicInteger(10)::incrementAndGet);
+ Schema expectedSchema =
+ new Schema(required(11, "v", testType), required(12, "A", Types.IntegerType.get()));
+ assertThat(assignedSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ Schema reassignedSchema = TypeUtil.reassignIds(schema, sourceSchema);
+ assertThat(reassignedSchema.asStruct()).isEqualTo(sourceSchema.asStruct());
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testIndexByIdWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Map<Integer, Types.NestedField> indexByIds = TypeUtil.indexById(schema.asStruct());
+ assertThat(indexByIds.get(0).type()).isEqualTo(testType);
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testIndexNameByIdWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Map<Integer, String> indexNameByIds = TypeUtil.indexNameById(schema.asStruct());
+ assertThat(indexNameByIds.get(0)).isEqualTo("v");
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testProjectWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+
+ Schema expectedSchema = new Schema(Lists.newArrayList(required(0, "v", testType)));
+ Schema projectedSchema = TypeUtil.project(schema, Sets.newHashSet(0));
+ assertThat(projectedSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
+
+ Set<Integer> projectedIds = TypeUtil.getProjectedIds(schema);
Review Comment:
It looks like this is testing both `getProjectedIds` and `project`. It's minor, but I think these should test one visitor at a time.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1929307357
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -24,38 +24,43 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.util.Set;
+import java.util.stream.Stream;
import org.apache.iceberg.Schema;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
-import org.apache.iceberg.types.Types.IntegerType;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
public class TestTypeUtil {
- @Test
- public void testReassignIdsDuplicateColumns() {
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(Arguments.of(Types.IntegerType.get()), Arguments.of(Types.VariantType.get()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsDuplicateColumns(Type testType) {
Review Comment:
I was trying to add the coverage for Variant changes for the visitors such as ReassignIds or PruneColumns with the existing tests. I guess what you suggest with some additional tests should be able to provide coverage for them. Let me look into that.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1929488493
##########
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) {
Review Comment:
Instead of adding new `typeFromTypeString()`, I change `fromPrimitiveString()` to `fromTypeString()` to avoid confusion. Let me know if that works for you.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942077666
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +647,37 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ @Test
+ public void testVariantType() {
+ Schema schema =
+ new Schema(
+ required(0, "v", Types.VariantType.get()), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(
+ required(1, "v", Types.VariantType.get()), required(2, "A", Types.IntegerType.get()));
+
+ Schema assignedSchema =
+ TypeUtil.assignFreshIds(sourceSchema, new AtomicInteger(10)::incrementAndGet);
+ Schema expectedSchema =
+ new Schema(
+ required(11, "v", Types.VariantType.get()), required(12, "A", Types.IntegerType.get()));
+ assertThat(assignedSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
+
+ final Schema reassignedSchema = TypeUtil.reassignIds(schema, sourceSchema);
+ assertThat(reassignedSchema.asStruct()).isEqualTo(sourceSchema.asStruct());
+
+ expectedSchema = new Schema(Lists.newArrayList(required(1, "v", Types.VariantType.get())));
+ Schema projectedSchema = TypeUtil.project(sourceSchema, Sets.newHashSet(1));
Review Comment:
I think that projection should be a different test case.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942078866
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +647,37 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ @Test
+ public void testVariantType() {
Review Comment:
I think this should be more specific. Right now, it looks like all of the methods that needed to be validated were added here (I see at least `assignFreshIds` and `project`).
Instead, I think that this should locate a test case with each group of tests (for the different `TestUtil` methods) and name it something like `testProjectWithVariantType()` or `testAssignFreshIdsWithVariantType()`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1960095348
##########
api/src/main/java/org/apache/iceberg/types/PrimitiveHolder.java:
##########
@@ -33,6 +33,6 @@ class PrimitiveHolder implements Serializable {
}
Object readResolve() throws ObjectStreamException {
- return Types.fromPrimitiveString(typeAsString);
+ return Types.fromTypeName(typeAsString);
Review Comment:
Looks like the test for this is missing variant.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942092261
##########
core/src/main/java/org/apache/iceberg/avro/BuildAvroProjection.java:
##########
@@ -56,6 +56,10 @@ class BuildAvroProjection extends AvroCustomOrderSchemaVisitor<Schema, Schema.Fi
@Override
@SuppressWarnings("checkstyle:CyclomaticComplexity")
public Schema record(Schema record, List<String> names, Iterable<Schema.Field> schemaIterable) {
+ if (current.isVariantType()) {
Review Comment:
If we are updating the visitor implementations, I think we should also update the visit method. We can also delay making thes changes.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894519023
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -52,6 +56,15 @@ public class TestMetadataUpdateParser {
Types.NestedField.required(1, "id", Types.IntegerType.get()),
Types.NestedField.optional(2, "data", Types.StringType.get()));
+ private static final Schema ID_VARIANTDATA_SCHEMA =
Review Comment:
This isn't the right place for a Variant test. What are you trying to test?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894518245
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -132,6 +133,8 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws
static void toJson(Type type, JsonGenerator generator) throws IOException {
if (type.isPrimitiveType()) {
toJson(type.asPrimitiveType(), generator);
+ } else if (type.isVariantType()) {
+ generator.writeString(type.toString());
Review Comment:
Can you update `TestSchemaParser`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "shohamyamin (via GitHub)" <gi...@apache.org>.
shohamyamin commented on PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#issuecomment-2571424964
@aihuaxu very important feature that will allow a lot more options for iceberg, thank you for your contribution
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894551188
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -42,6 +42,7 @@ 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";
Review Comment:
I was thinking of that. But PrimitiveHolder calls fromPrimitiveString() which is used in PrimitiveType class. Not sure if it would cause an issue in serialization.
> /** Replacement for primitive types in Java Serialization. */
> class PrimitiveHolder implements Serializable {
> private String typeAsString = null;
> ...
> Object readResolve() throws ObjectStreamException {
> return Types.fromPrimitiveString(typeAsString);
> }
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894551275
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -52,6 +56,15 @@ public class TestMetadataUpdateParser {
Types.NestedField.required(1, "id", Types.IntegerType.get()),
Types.NestedField.optional(2, "data", Types.StringType.get()));
+ private static final Schema ID_VARIANTDATA_SCHEMA =
Review Comment:
I'm trying to test SchemaParser indirectly. I don't see TestSchemaParser.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955196818
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
Looks like this is not yet implemented by:
* `CheckCompatibility` (tested by `TestReadabilityChecks`)
* `FixupTypes`, `SparkFixupTypes`, or `FlinkFixupTypes`
* `PruneColumnsWithoutReordering` (Spark, so maybe this can be done separately)
* `ReassignDoc`
* `AssignIds`
It is also not implemented by `PruneColumnsWithReordering` but that doesn't appear to be referenced anywhere. I opened #12258 to remove these.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955170959
##########
core/src/test/java/org/apache/iceberg/avro/TestSchemaConversions.java:
##########
@@ -370,4 +370,22 @@ public void testFieldDocsArePreserved() {
Lists.newArrayList(Iterables.transform(origSchema.columns(), Types.NestedField::doc));
assertThat(fieldDocs).isEqualTo(origFieldDocs);
}
+
+ @Test
+ public void testVariantConversion() {
Review Comment:
I think this should also be in #12238 rather than here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953613423
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +651,77 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ private static Stream<Arguments> testTypes() {
Review Comment:
Thank you! This is great.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953676388
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
I missed that.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1960134459
##########
api/src/test/java/org/apache/iceberg/types/TestSerializableTypes.java:
##########
@@ -46,6 +46,7 @@ public void testIdentityTypes() throws Exception {
Types.StringType.get(),
Types.UUIDType.get(),
Types.BinaryType.get(),
+ Types.UnknownType.get()
Review Comment:
Variant test is already added below. Since for variant, we need to test with isEqualTo() instead of isSameAs().
> public void testVariant() throws Exception {
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927936162
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -108,19 +121,19 @@ public void testUpgradeFormatVersionFromJson() {
}
/** AddSchema * */
- @Test
- public void testAddSchemaFromJson() {
+ @ParameterizedTest
+ @MethodSource("testSchemas")
Review Comment:
This also abuses existing test cases to test different behavior than was intended. I think tests should be written directly instead.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1935955729
##########
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) {
Review Comment:
Actually it will introduces incompatibility since the return type changes. Let me know if that works or maybe we can introduce a new one as I did initially.
> old: method org.apache.iceberg.types.Type.PrimitiveType org.apache.iceberg.types.Types::fromPrimitiveString(java.lang.String)
> new: method org.apache.iceberg.types.Type org.apache.iceberg.types.Types::fromPrimitiveString(java.lang.String)
>
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894518424
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -42,6 +42,7 @@ 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";
Review Comment:
Types don't typically go here. Maybe add this to `fromPrimitiveString`. It fits better there, even if it isn't a primitive.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#issuecomment-2555947458
cc @rdblue, @RussellSpitzer, @flyrain and @JonasJ-ap. This is to add the changes in core to support variant type.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953621390
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +651,77 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(
+ Arguments.of(Types.UnknownType.get()),
+ Arguments.of(Types.VariantType.get()),
+ Arguments.of(Types.TimestampNanoType.withoutZone()),
+ Arguments.of(Types.TimestampNanoType.withZone()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignFreshIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ Schema assignedSchema =
+ TypeUtil.assignFreshIds(sourceSchema, new AtomicInteger(10)::incrementAndGet);
+ Schema expectedSchema =
+ new Schema(required(11, "v", testType), required(12, "A", Types.IntegerType.get()));
+ assertThat(assignedSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ final Schema reassignedSchema = TypeUtil.reassignIds(schema, sourceSchema);
+ assertThat(reassignedSchema.asStruct()).isEqualTo(sourceSchema.asStruct());
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testIndexByIdWithType(Type testType) {
+ Schema schema =
Review Comment:
This is another case where `schema` isn't used. Could you clean these up? The `schema` variable should be used when no `sourceSchema` is needed for ID assignment. And we don't need extra variables.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953614605
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +651,77 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(
+ Arguments.of(Types.UnknownType.get()),
+ Arguments.of(Types.VariantType.get()),
+ Arguments.of(Types.TimestampNanoType.withoutZone()),
+ Arguments.of(Types.TimestampNanoType.withZone()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignFreshIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ Schema assignedSchema =
+ TypeUtil.assignFreshIds(sourceSchema, new AtomicInteger(10)::incrementAndGet);
+ Schema expectedSchema =
+ new Schema(required(11, "v", testType), required(12, "A", Types.IntegerType.get()));
+ assertThat(assignedSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ final Schema reassignedSchema = TypeUtil.reassignIds(schema, sourceSchema);
Review Comment:
Style: avoid `final` for local variables. The only reason to use it is when the value needs to be passed into a context that requires a `final` reference. But I'm not sure that's even required anymore because it is almost always detected automatically.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953612378
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant() {
+ return null;
Review Comment:
Looks like this is still null. The one above is updated though.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953671095
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +651,77 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(
+ Arguments.of(Types.UnknownType.get()),
+ Arguments.of(Types.VariantType.get()),
+ Arguments.of(Types.TimestampNanoType.withoutZone()),
+ Arguments.of(Types.TimestampNanoType.withZone()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignFreshIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ Schema assignedSchema =
+ TypeUtil.assignFreshIds(sourceSchema, new AtomicInteger(10)::incrementAndGet);
Review Comment:
You are right. I just need one schema. I may not get the following: seems we can test for unknown and other primitive types here as well to make sure the correct type is returned, not null.
> we only need on test with a variant here. Other tests with more logic for primitives may need more cases though.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955196818
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
Looks like this is not yet implemented by:
* `CheckCompatibility` (tested by `TestReadabilityChecks`)
* `FixupTypes`, `SparkFixupTypes`, or `FlinkFixupTypes`
* `PruneColumnsWithoutReordering`
* `ReassignDoc`
* `AssignIds`
It is also not implemented by `PruneColumnsWithReordering` but that doesn't appear to be referenced anywhere. I opened #12258 to remove these.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1933336931
##########
core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java:
##########
@@ -49,6 +49,15 @@ abstract class TypeToSchema extends TypeUtil.SchemaVisitor<Schema> {
private static final Schema UUID_SCHEMA =
LogicalTypes.uuid().addToSchema(Schema.createFixed("uuid_fixed", null, null, 16));
private static final Schema BINARY_SCHEMA = Schema.create(Schema.Type.BYTES);
+ private static final Schema VARIANT_SCHEMA =
Review Comment:
You are right. We need to have different name for each record, otherwise, it will generate only one record.
I generate a new record with the name generated from field ID now. I don't cache it since the type is the same.
This is the schema with two variant columns now:
`{"name":"_v1","type":["null",{"type":"record","name":"r18","fields":[{"name":"metadata","type":"bytes"},{"name":"value","type":"bytes"}]}],"default":null,"field-id":18},{"name":"_v2","type":["null",{"type":"record","name":"r19","fields":[{"name":"metadata","type":"bytes"},{"name":"value","type":"bytes"}]}],"default":null,"field-id":19}]}`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1958660466
##########
api/src/main/java/org/apache/iceberg/types/CheckCompatibility.java:
##########
@@ -250,6 +250,19 @@ public List<String> map(
}
}
+ @Override
+ public List<String> variant(Types.VariantType readVariant) {
+ if (currentType.isVariantType()) {
+ return NO_ERRORS;
+ }
+
+ // Currently promotion is not allowed to variant type
+ return ImmutableList.of(
+ String.format(
+ ": %s cannot be read as a %s",
+ currentType.typeId().toString().toLowerCase(Locale.ENGLISH), readVariant));
Review Comment:
Nit: `currentType` should work instead of this conversion. I know this was copied from primitive, but it isn't needed.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927920618
##########
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) {
Review Comment:
Variant isn't a primitive, but it is still better to reuse `fromPrimitiveString`. Callers are using this to parse type names, not to restrict parsing to only primitive types.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927926305
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -85,33 +89,33 @@ public void testAssignIncreasingFreshIdWithIdentifier() {
.isEqualTo(expectedSchema.identifierFieldIds());
}
- @Test
- public void testAssignIncreasingFreshIdNewIdentifier() {
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignIncreasingFreshIdNewIdentifier(Type testType) {
Review Comment:
Please roll back changes to this test.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#issuecomment-2666701335
@aihuaxu @rdblue An unfortunate collision, created a fix here: https://github.com/apache/iceberg/pull/12316
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942081865
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -42,6 +42,7 @@ 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";
Review Comment:
Ah, thanks for pointing out that `fromPrimitiveString` returns `PrimitiveType`. That probably won't work then.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942101398
##########
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 don't think that this is going to exercise the path that we want to when the visitor and visit method are updated. This should create a record schema and validate that visiting a file schema gives the correct projection to request, by name.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894551476
##########
core/src/test/java/org/apache/iceberg/avro/TestAvroSchemaProjection.java:
##########
@@ -150,4 +152,16 @@ public void projectWithMapSchemaChanged() {
.as("Result of buildAvroProjection is missing some IDs")
.isFalse();
}
+
+ @Test
+ public void testVariantConversion() {
Review Comment:
You are right. Let me move there.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927926035
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -24,38 +24,43 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import java.util.Set;
+import java.util.stream.Stream;
import org.apache.iceberg.Schema;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
-import org.apache.iceberg.types.Types.IntegerType;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
public class TestTypeUtil {
- @Test
- public void testReassignIdsDuplicateColumns() {
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(Arguments.of(Types.IntegerType.get()), Arguments.of(Types.VariantType.get()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsDuplicateColumns(Type testType) {
Schema schema =
- new Schema(
- required(0, "a", Types.IntegerType.get()), required(1, "A", Types.IntegerType.get()));
+ new Schema(required(0, "a", testType), required(1, "A", Types.IntegerType.get()));
Schema sourceSchema =
- new Schema(
- required(1, "a", Types.IntegerType.get()), required(2, "A", Types.IntegerType.get()));
+ new Schema(required(1, "a", testType), required(2, "A", Types.IntegerType.get()));
final Schema actualSchema = TypeUtil.reassignIds(schema, sourceSchema);
assertThat(actualSchema.asStruct()).isEqualTo(sourceSchema.asStruct());
}
- @Test
- public void testReassignIdsWithIdentifier() {
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testReassignIdsWithIdentifier(Type testType) {
Review Comment:
Please roll back changes to this test.
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -64,19 +69,18 @@ public void testReassignIdsWithIdentifier() {
.isEqualTo(sourceSchema.identifierFieldIds());
}
- @Test
- public void testAssignIncreasingFreshIdWithIdentifier() {
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignIncreasingFreshIdWithIdentifier(Type testType) {
Review Comment:
Please roll back changes to this test.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1927926739
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -85,33 +89,33 @@ public void testAssignIncreasingFreshIdWithIdentifier() {
.isEqualTo(expectedSchema.identifierFieldIds());
}
- @Test
- public void testAssignIncreasingFreshIdNewIdentifier() {
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignIncreasingFreshIdNewIdentifier(Type testType) {
Review Comment:
@aihuaxu, it looks like I'd be pasting this message a lot. Please roll back all changes to this test suite and add new behavior tests to exercise the relevant behavior for variants.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942103932
##########
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:
Why is this test in `TestGenericData`? It looks like this is testing the output of `convert(schema, "table")` but passing the schema through `GenericData.Record` and a `GenericRecordBuilder` first?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955155426
##########
core/src/main/java/org/apache/iceberg/schema/SchemaWithPartnerVisitor.java:
##########
@@ -106,7 +106,8 @@ public static <P, T> T visit(
}
return visitor.map(map, partner, keyResult, valueResult);
-
Review Comment:
Nit: unnecessary whitespace 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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953620434
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -645,4 +651,77 @@ public void testReassignOrRefreshIdsCaseInsensitive() {
required(2, "FIELD2", Types.IntegerType.get())));
assertThat(actualSchema.asStruct()).isEqualTo(expectedSchema.asStruct());
}
+
+ private static Stream<Arguments> testTypes() {
+ return Stream.of(
+ Arguments.of(Types.UnknownType.get()),
+ Arguments.of(Types.VariantType.get()),
+ Arguments.of(Types.TimestampNanoType.withoutZone()),
+ Arguments.of(Types.TimestampNanoType.withZone()));
+ }
+
+ @ParameterizedTest
+ @MethodSource("testTypes")
+ public void testAssignFreshIdsWithType(Type testType) {
+ Schema schema =
+ new Schema(required(0, "v", testType), required(1, "A", Types.IntegerType.get()));
+ Schema sourceSchema =
+ new Schema(required(1, "v", testType), required(2, "A", Types.IntegerType.get()));
+
+ Schema assignedSchema =
+ TypeUtil.assignFreshIds(sourceSchema, new AtomicInteger(10)::incrementAndGet);
Review Comment:
This doesn't use `sourceSchema` as intended. ID assignment with a source schema uses assigned IDs from that schema when names match and assigns missing/new IDs using the `NextID` supplier. When using the single schema version, the second schema should not be present.
That also raises the question about what we need to test for new types. In this case, because the type just needs to be handled by `field` and `struct`, I think it's fine to just have a case that validates that `UnsupportedOperationException` is not thrown and that the behavior is expected. That is, we only need on test with a variant here. Other tests with more logic for primitives may need more cases though.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955206561
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -616,8 +616,8 @@ public T map(Types.MapType map, T keyResult, T valueResult) {
return null;
}
- public T variant() {
- return null;
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
This still need to be implemented by:
* `CreateMapping`
* `DescribeSchemaVisitor`
* `IdToOrcName` and other conversions
I think it's fine to implement the conversions later as part of Avro, Parquet, and ORC work, but the common ones should probably be implemented.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1929379839
##########
core/src/main/java/org/apache/iceberg/avro/BuildAvroProjection.java:
##########
@@ -56,6 +56,10 @@ class BuildAvroProjection extends AvroCustomOrderSchemaVisitor<Schema, Schema.Fi
@Override
@SuppressWarnings("checkstyle:CyclomaticComplexity")
public Schema record(Schema record, List<String> names, Iterable<Schema.Field> schemaIterable) {
+ if (current.isVariantType()) {
Review Comment:
You are right. This is to workaround before the logical type is added in Avro by using Iceberg variant type. Let me add a comment for this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955552361
##########
core/src/test/java/org/apache/iceberg/avro/TestSchemaConversions.java:
##########
@@ -370,4 +370,22 @@ public void testFieldDocsArePreserved() {
Lists.newArrayList(Iterables.transform(origSchema.columns(), Types.NestedField::doc));
assertThat(fieldDocs).isEqualTo(origFieldDocs);
}
+
+ @Test
+ public void testVariantConversion() {
Review Comment:
Let me move there. Initially thought we had to add variant in Avro project so try to have all of the changes here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1933336931
##########
core/src/main/java/org/apache/iceberg/avro/TypeToSchema.java:
##########
@@ -49,6 +49,15 @@ abstract class TypeToSchema extends TypeUtil.SchemaVisitor<Schema> {
private static final Schema UUID_SCHEMA =
LogicalTypes.uuid().addToSchema(Schema.createFixed("uuid_fixed", null, null, 16));
private static final Schema BINARY_SCHEMA = Schema.create(Schema.Type.BYTES);
+ private static final Schema VARIANT_SCHEMA =
Review Comment:
You are right. We need to have different name for each record, otherwise, it will generate only one record.
I generate a new record with the name generated from field ID now. I don't cache it since the type is the same.
This is the schema with two variant columns:
`{"name":"_v1","type":["null",{"type":"record","name":"r18","fields":[{"name":"metadata","type":"bytes"},{"name":"value","type":"bytes"}]}],"default":null,"field-id":18},{"name":"_v2","type":["null",{"type":"record","name":"r19","fields":[{"name":"metadata","type":"bytes"},{"name":"value","type":"bytes"}]}],"default":null,"field-id":19}]}`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1935955729
##########
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) {
Review Comment:
Actually it will introduces incompatibility since the return type changes. Let me know if that works or maybe we can introduce a new one `fromTypeString()` as I did initially.
> old: method org.apache.iceberg.types.Type.PrimitiveType org.apache.iceberg.types.Types::fromPrimitiveString(java.lang.String)
> new: method org.apache.iceberg.types.Type org.apache.iceberg.types.Types::fromPrimitiveString(java.lang.String)
>
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942081865
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -42,6 +42,7 @@ 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";
Review Comment:
Ah, thanks for pointing out that `fromPrimitiveString` returns `PrimitiveType`. That won't work then.
We do want places like `PrimitiveHolder` to work with Variant, since it is an unparameterized type. We should be using it to serialize and maintain `VariantType` as a singleton. I'd probably update it to use `fromString` and rename it if you think it is confusing (It could be `UnparameterizedTypeHolder`, but it isn't a big deal because it is package-private).
For the new method, I think we need to find a good name. `fromString` is possible, but it doesn't capture that `list`, `struct`, and `map` types aren't supported. What about `fromTypeName`? Then it is maybe a little more clear that you can't construct a `struct` type from just a name?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1929433492
##########
core/src/test/java/org/apache/iceberg/avro/TestBuildAvroProjection.java:
##########
@@ -401,4 +402,32 @@ 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")
+ .namespace("unit.test")
+ .fields()
+ .name("metadata")
Review Comment:
You are right. variant `value` and `metadata` should not be assigned IDs.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1935939182
##########
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) {
Review Comment:
It's public API and renaming actually will cause incompatibility. I renamed back to `fromPrimitiveString()` now.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1936176654
##########
api/src/main/java/org/apache/iceberg/transforms/Identity.java:
##########
@@ -39,7 +42,7 @@ class Identity<T> implements Transform<T, T> {
@Deprecated
public static <I> Identity<I> get(Type type) {
Preconditions.checkArgument(
- type.typeId() != Type.TypeID.VARIANT, "Unsupported type for identity: %s", type);
+ !UNSUPPORTED_TYPES.contains(type), "Unsupported type for identity: %s", type);
Review Comment:
This comment was super old (I guess I forgot to publish), and it looks like it ended up being addresed. Resolving
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1937753339
##########
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();
+ 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();
Review Comment:
I changed to check the type now so it will test it's bytes field.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1937756284
##########
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) {
Review Comment:
Maybe it's better just to change the method to `fromTypeString`() instead of introducing a new one to keep the api simple. Please take another look.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1934325991
##########
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) {
Review Comment:
I'm thinking that `fromPrimitiveString` is a little misleading so prefer `fromTypeString`. But let me know and I can change back.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955196818
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
Looks like this is not yet implemented by:
* `CheckCompatibility`
* `FixupTypes`, `SparkFixupTypes`, or `FlinkFixupTypes`
* `PruneColumnsWithoutReordering`
* `ReassignDoc`
* `AssignIds`
It is also not implemented by `PruneColumnsWithReordering` but that doesn't appear to be referenced anywhere. I opened #12258 to remove these.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1955196818
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -712,6 +712,10 @@ public T map(Types.MapType map, Supplier<T> keyResult, Supplier<T> valueResult)
return null;
}
+ public T variant(Types.VariantType variant) {
+ throw new UnsupportedOperationException("Unsupported type: variant");
Review Comment:
Looks like this is not yet implemented by `CheckCompatibility`, `FixupTypes`, or `FlinkFixupTypes`.
It is also not implemented by `PruneColumnsWithReordering` but that doesn't appear to be referenced anywhere. I opened #12258 to remove these.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #11831:
URL: https://github.com/apache/iceberg/pull/11831
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#issuecomment-2666325705
@aihuaxu It looks like we have a sad CI on main: https://github.com/apache/iceberg/actions/runs/13395385725/job/37413381567
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1894551275
##########
core/src/test/java/org/apache/iceberg/TestMetadataUpdateParser.java:
##########
@@ -52,6 +56,15 @@ public class TestMetadataUpdateParser {
Types.NestedField.required(1, "id", Types.IntegerType.get()),
Types.NestedField.optional(2, "data", Types.StringType.get()));
+ private static final Schema ID_VARIANTDATA_SCHEMA =
Review Comment:
There is no TestSchemaParser. I'm trying to test SchemaParser indirectly. Let me know if we should add TestSchemaParser or we don't have that because it's tested indirectly.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942096627
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -75,7 +75,7 @@ private static NestedField[] primitiveFields(
optional(
atomicInteger.incrementAndGet(),
type.toString(),
- Types.fromPrimitiveString(type.toString())))
+ Types.fromTypeString(type.toString())))
Review Comment:
It looks like this is correct, but do you want to add `VariantType.get()` to the list of types that are tested as well?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1942090932
##########
core/src/main/java/org/apache/iceberg/SchemaParser.java:
##########
@@ -145,6 +145,8 @@ static void toJson(Type.PrimitiveType primitive, JsonGenerator generator) throws
static void toJson(Type type, JsonGenerator generator) throws IOException {
if (type.isPrimitiveType()) {
toJson(type.asPrimitiveType(), generator);
+ } else if (type.isVariantType()) {
+ generator.writeString(type.toString());
Review Comment:
Minor: I'd probably just inline the `toJson` call from the previous case along with this:
```java
if (type.isPrimitiveType() || type.isVariantType()) {
generator.writeString(type.toString());
} else {
```
The idea is just to have one case that does the same thing that's easier to read. The `toJson` call currently used isn't needed and is only used in that one place. We could also deprecate it but I don't think it matters.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#discussion_r1953613226
##########
api/src/main/java/org/apache/iceberg/types/TypeUtil.java:
##########
@@ -788,6 +792,9 @@ public static <T> T visit(Type type, CustomOrderSchemaVisitor<T> visitor) {
new VisitFuture<>(map.keyType(), visitor),
new VisitFuture<>(map.valueType(), visitor));
+ case VARIANT:
+ return visitor.variant();
Review Comment:
I'm debating whether this should pass the type. If we want to do it later, that would be an added method. When returning types, it's a good idea to pass the original instance, rather than assuming that it is identical.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: add variant type support [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on PR #11831:
URL: https://github.com/apache/iceberg/pull/11831#issuecomment-2670407909
> @aihuaxu It looks like we have a sad CI on main: https://github.com/apache/iceberg/actions/runs/13395385725/job/37413381567
Oh. Seems it conflicts with #12302. @Fokko Thanks for fixing it.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org