Parquet: Implement Variant writers #12323

rdblue · 2025-02-18T23:30:49Z

This PR implements Variant writers for Parquet based on a Parquet schema passed into the writer builder. It works basically the same as #12139.

rdblue · 2025-02-20T23:44:01Z

core/src/main/java/org/apache/iceberg/variants/Variants.java

@@ -85,6 +85,16 @@ public static ShreddedObject object(VariantMetadata metadata) {
    return new ShreddedObject(metadata);
  }

+  public static ShreddedObject object(VariantObject object) {


Used to create a shredded object from an existing object when writing. It uses the object's metadata.

This avoids exposing VariantObject.metadata because metadata is carried by Variant instead of values.

rdblue · 2025-02-20T23:45:19Z

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetWriter.java

@@ -62,7 +71,7 @@ protected ParquetValueWriter<?> timestampWriter(ColumnDescriptor desc, boolean i
    }
  }

-  private class WriteBuilder extends ParquetTypeVisitor<ParquetValueWriter<?>> {
+  private class WriteBuilder extends TypeWithSchemaVisitor<ParquetValueWriter<?>> {


In order to detect a variant type, this needs to use the original Iceberg schema. When Parquet exposes the VARIANT logical type annotation, we can update this to no longer require the schema.

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

rdblue · 2025-02-20T23:48:01Z

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

    private String name = "table";
    private WriteSupport<?> writeSupport = null;
-    private Function<MessageType, ParquetValueWriter<?>> createWriterFunc = null;
+    private BiFunction<Schema, MessageType, ParquetValueWriter<?>> createWriterFunc = null;


This change is supports passing the schema to the write builder function.

rdblue · 2025-02-20T23:48:40Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java

@@ -71,6 +71,10 @@ public static UnboxedWriter<Short> shorts(ColumnDescriptor desc) {
    return new ShortWriter(desc);
  }

+  public static <T> ParquetValueWriter<T> unboxed(ColumnDescriptor desc) {


Needed to expose this to get float and double working. Those writers currently require a metrics builder that expects a non-null field ID.

rdblue · 2025-02-21T00:56:23Z

core/src/main/java/org/apache/iceberg/variants/VariantData.java

+  private final VariantMetadata metadata;
+  private final VariantValue value;
+
+  VariantData(VariantMetadata metadata, VariantValue value) {


This was originally in #12304, but I moved it here after reverting changes to variant classes in that PR. This uses it to create objects that are passed to writers and created by readers.

rdblue · 2025-02-21T23:18:22Z

Rebased after moving variants to API in #12374.

aihuaxu · 2025-02-26T22:51:00Z

core/src/main/java/org/apache/iceberg/variants/VariantVisitor.java

+package org.apache.iceberg.variants;
+
+import java.util.List;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;


I think the same as variant reader PR, we are not including array, right?

That's right.

core/src/main/java/org/apache/iceberg/variants/VariantVisitor.java

aihuaxu · 2025-02-26T23:51:22Z

parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetWriter.java

+    }
+
+    @Override
+    public ParquetVariantVisitor<ParquetValueWriter<?>> variantVisitor() {


We can reduce the scope to protected for now. It doesn't seem to be accessed outside.

This is part of the Parquet visitor contract now. It needs to be public because that class is public.

aihuaxu · 2025-02-27T02:19:55Z

parquet/src/main/java/org/apache/iceberg/parquet/VariantWriterBuilder.java

+  @Override
+  public ParquetValueWriter<?> value(
+      GroupType value, ParquetValueWriter<?> valueWriter, ParquetValueWriter<?> typedWriter) {
+    int valueDL = schema.getMaxDefinitionLevel(path(VALUE));


Rename this to valueDefinitionLevel to be consistent.

aihuaxu · 2025-02-27T05:41:58Z

parquet/src/main/java/org/apache/iceberg/parquet/ParquetVariantWriters.java

+  }
+
+  static ParquetValueWriter<VariantValue> primitive(
+      ParquetValueWriter<?> writer, PhysicalType... types) {


We accept varargs because of BOOLEAN_TRUE or BOOLEAN_FALSE and we don't expect multiple types?

Can we add a check here to make sure only one type is passed other than boolean?

This may be used for other cases in the future. I don't see a reason to limit it right now.

aihuaxu · 2025-02-27T05:52:45Z

parquet/src/main/java/org/apache/iceberg/parquet/TypeToMessageType.java

@@ -56,6 +58,19 @@ public class TypeToMessageType {
      LogicalTypeAnnotation.timestampType(false /* not adjusted to UTC */, TimeUnit.MICROS);
  private static final LogicalTypeAnnotation TIMESTAMPTZ_MICROS =
      LogicalTypeAnnotation.timestampType(true /* adjusted to UTC */, TimeUnit.MICROS);
+  private static final String METADATA = "metadata";


Right now we are defining the constants in multiple places. We don't want to be in API but does it make sense in core Variants.java?

I'm not really sure yet, so I've made them all private or package-private so we can look at where they should live later.

aihuaxu

LGTM.

danielcweeks · 2025-03-04T20:52:52Z

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

@@ -160,9 +161,10 @@ public static class WriteBuilder implements InternalData.WriteBuilder {
    private final Map<String, String> metadata = Maps.newLinkedHashMap();
    private final Map<String, String> config = Maps.newLinkedHashMap();
    private Schema schema = null;
+    private BiFunction<Integer, String, Type> variantShreddingFunc = null;


I feel less and less comfortable about using functional references like this in public interfaces. This really makes maintainability much harder because we're not documenting what this represents. If we're going have a reference like this, then we should just make an wrapper interface (e.g. VariantShreddingFunction) that documents the parameters/return and allows for better navigation of usage.

This is particularly prevalent in parquet and avro, but it even here we have three different functional references that we're passing around. If it were internal, I'd be fine, but these are public apis so we should do better.

You're right. I think this should have been a private constructor and this should only be available through the ParquetSchemaUtil method that documents how it is used. I'll update it.

I updated the config method to document this and explain what is happening.

Updated to use VariantShreddingFunction.

rdblue · 2025-03-05T22:50:57Z

Thanks for the reviews, @danielcweeks and @aihuaxu!

github-actions bot added parquet core labels Feb 18, 2025

rdblue force-pushed the variant-parquet-writers branch 3 times, most recently from 360f531 to 086a16c Compare February 20, 2025 23:38

rdblue commented Feb 20, 2025

View reviewed changes

parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java Outdated Show resolved Hide resolved

rdblue commented Feb 20, 2025

View reviewed changes

rdblue commented Feb 21, 2025

View reviewed changes

rdblue mentioned this pull request Feb 21, 2025

API: Move Variant interfaces and serialized implementations to API #12374

Merged

rdblue closed this Feb 21, 2025

rdblue reopened this Feb 21, 2025

rdblue force-pushed the variant-parquet-writers branch from 67c2bab to f4296c3 Compare February 21, 2025 23:18

github-actions bot added the API label Feb 21, 2025

rdblue force-pushed the variant-parquet-writers branch from f4296c3 to f3e3ccc Compare February 21, 2025 23:20

aihuaxu reviewed Feb 27, 2025

View reviewed changes

aihuaxu approved these changes Mar 1, 2025

View reviewed changes

danielcweeks reviewed Mar 4, 2025

View reviewed changes

rdblue added 6 commits March 4, 2025 19:05

Parquet: Implement Variant writers.

e93c910

Add tests for Parquet variant writers.

d273904

Add VariantData implementation of Variant.

7feb172

Pass field names to object method.

03ce5d8

Parquet: Document the variant shredding function.

e69ec6a

Pull out VariantShreddingFunction interface.

4bcf388

rdblue force-pushed the variant-parquet-writers branch from dc141c8 to 4bcf388 Compare March 5, 2025 03:15

Apply spotless.

36a224a

danielcweeks approved these changes Mar 5, 2025

View reviewed changes

danielcweeks merged commit 58b283e into apache:main Mar 5, 2025
43 checks passed

aihuaxu mentioned this pull request Mar 7, 2025

Parquet: Support Variant Array read and write #12472

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parquet: Implement Variant writers #12323

Parquet: Implement Variant writers #12323

rdblue commented Feb 18, 2025 •

edited

Loading

rdblue Feb 20, 2025

rdblue Feb 20, 2025

rdblue Feb 20, 2025

rdblue Feb 20, 2025

rdblue Feb 21, 2025

rdblue commented Feb 21, 2025

aihuaxu Feb 26, 2025

rdblue Feb 27, 2025

aihuaxu Feb 26, 2025

rdblue Mar 1, 2025

aihuaxu Feb 27, 2025

aihuaxu Feb 27, 2025

rdblue Mar 1, 2025

aihuaxu Feb 27, 2025

rdblue Mar 1, 2025

aihuaxu left a comment

danielcweeks Mar 4, 2025

rdblue Mar 4, 2025

rdblue Mar 4, 2025

rdblue Mar 5, 2025

rdblue commented Mar 5, 2025

Parquet: Implement Variant writers #12323

Parquet: Implement Variant writers #12323

Conversation

rdblue commented Feb 18, 2025 • edited Loading

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

rdblue commented Feb 21, 2025

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 left a 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

rdblue commented Mar 5, 2025

rdblue commented Feb 18, 2025 •

edited

Loading