Core: add variant type support #11831

aihuaxu · 2024-12-19T22:59:37Z

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.

Part of: #10392

aihuaxu · 2024-12-19T23:25:51Z

cc @rdblue, @RussellSpitzer, @flyrain and @JonasJ-ap. This is to add the changes in core to support variant type.

rdblue · 2024-12-20T22:14:17Z

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);


Why change this? It works just fine right? We can alter this when we add another type.

Yes. It works fine. I thought you mentioned that you wanted to make such change in a previous PR. :)

rdblue · 2024-12-21T00:33:32Z

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;


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.

I can see others like map()/list() all return null here.

rdblue · 2024-12-21T00:34:04Z

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());


Can you update TestSchemaParser?

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.

rdblue · 2024-12-21T00:34:52Z

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";


Types don't typically go here. Maybe add this to fromPrimitiveString. It fits better there, even if it isn't a primitive.

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);
}

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.

rdblue · 2024-12-21T00:35:14Z

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())) {


I think fromPrimitiveString should handle this.

rdblue · 2024-12-21T00:37:36Z

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 =


This isn't the right place for a Variant test. What are you trying to test?

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.

rdblue · 2024-12-21T00:38:02Z

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() {


Is the projection test suite the right place for conversion tests? Isn't there a conversion test suite?

You are right. Let me move there.

github-actions bot added API core labels Dec 19, 2024

aihuaxu marked this pull request as ready for review December 19, 2024 23:00

aihuaxu force-pushed the variant-type-core branch from 5a18496 to 68d690e Compare December 19, 2024 23:20

ebyhr mentioned this pull request Dec 20, 2024

Add support for variant type in Iceberg trinodb/trino#24538

Open

rdblue reviewed Dec 20, 2024

View reviewed changes

rdblue reviewed Dec 21, 2024

View reviewed changes

Core: add variant type support

4bcb86b

aihuaxu force-pushed the variant-type-core branch from 68d690e to 04958ca Compare December 21, 2024 04:43

aihuaxu requested a review from rdblue December 21, 2024 04:45

aihuaxu force-pushed the variant-type-core branch from 04958ca to b276d3f Compare December 21, 2024 15:49

Address comments

fe6038a

aihuaxu force-pushed the variant-type-core branch from b276d3f to fe6038a Compare December 21, 2024 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: add variant type support #11831

Core: add variant type support #11831

aihuaxu commented Dec 19, 2024 •

edited

Loading

aihuaxu commented Dec 19, 2024

rdblue Dec 20, 2024

aihuaxu Dec 20, 2024

rdblue Dec 21, 2024

aihuaxu Dec 21, 2024

rdblue Dec 21, 2024

aihuaxu Dec 21, 2024

rdblue Dec 21, 2024

aihuaxu Dec 21, 2024

aihuaxu Dec 21, 2024

rdblue Dec 21, 2024

rdblue Dec 21, 2024

aihuaxu Dec 21, 2024 •

edited

Loading

rdblue Dec 21, 2024

aihuaxu Dec 21, 2024

Core: add variant type support #11831

Are you sure you want to change the base?

Core: add variant type support #11831

Conversation

aihuaxu commented Dec 19, 2024 • edited Loading

aihuaxu commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aihuaxu Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aihuaxu commented Dec 19, 2024 •

edited

Loading

aihuaxu Dec 21, 2024 •

edited

Loading