Core: add variant builder implementation #11857

aihuaxu · 2024-12-22T20:07:51Z

This PR adds the implementation to build a variant for primitive, object and array.

The usage:

Build from Json
VariantBuilder.parseJson(json_string);
build a primitive

VariantBuilder builder = new VariantBuilder();
VariantPrimitiveBuilder primBuilder = builder.createPrimitive();
Variant variant = primBuilder.writeNumeric(10).build();

build an object

 VariantBuilder builder = new VariantBuilder();
 VariantObjectBuilder objBuilder = builder.startObject();
 objBuilder.writeString("key1", "value1").writeNumeric("key2", 3).endObject();
 Variant variant = builder.build();

build an array

 VariantBuilder builder = new VariantBuilder();
 VariantArrayBuilder arrBuilder = builder.createArray();
 arrBuilder.writerNumeric(10).writerNumeric(20).endArray();
 Variant variant = builder.build();

Nested arrays and objects can be supported as well. Please see the test cases for the usage. Compared to build from JSON string, we also support some additional types such as timestamp, date.

Part of: #10392

aihuaxu · 2025-01-06T22:42:26Z

@rdblue, @RussellSpitzer Please help review. Thanks.

rdblue · 2025-01-24T00:55:01Z

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

@@ -20,6 +20,10 @@

 /** An variant array value. */
 public interface VariantArray extends VariantValue {
+  default int numElements() {
+    throw new UnsupportedOperationException();


Has VariantArray been in a release?

Just checked. It's not in any release. What is the question for?

This has been added so I just rebased so we don't need this anymore.

rdblue · 2025-01-24T00:56:16Z

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

+ */
+package org.apache.iceberg.variants;
+
+public class VariantSizeLimitException extends RuntimeException {


Why is there an exception here? Do we need to limit the size of variants that are being constructed?

I think we had the discussion that we will not limit the size. Let me remove that.

rdblue · 2025-01-24T00:57:11Z

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

+  private final VariantMetadata metadata;
+  private final VariantValue value;
+
+  public VariantImpl(byte[] metadata, byte[] value) {


Variants should use ByteBuffer to avoid copying problems from byte[]. That's why the current implementation is built around ByteBuffer.

rdblue · 2025-01-24T00:58:06Z

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

+ */
+package org.apache.iceberg.variants;
+
+public class VariantConstants {


There are other constants in Variants. Why add a new class for these? Should we standardize where they live?

Ah. You intend to place the constants in Variants? That works.

rdblue · 2025-01-24T00:58:28Z

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

+
+  public static final int MAX_DECIMAL4_PRECISION = 9;
+  public static final int MAX_DECIMAL8_PRECISION = 18;
+  public static final int MAX_DECIMAL16_PRECISION = 38;


Do all of these constants need to be public? What is the intended use outside of this package?

I refactored to VariantBuilderBase since they are only used there.

rdblue · 2025-01-24T01:01:46Z

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

+    Preconditions.checkArgument(
+        json != null && !json.isEmpty(), "Input JSON string cannot be null or empty.");
+
+    try (JsonParser parser = new JsonFactory().createParser(json)) {


There are existing utilities in JSONUtil for a lot of the boilerplate around parsing and generating JSON. Is there a reason not to use the same approach here?

We need to parse the JSON token by token so we can encode them in binary. Seems we only have the version to parse the JSON string into a full JSONNode and then create the object.

rdblue · 2025-01-24T01:03:22Z

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

+  /**
+   * Attempts to parse a JSON number as a decimal and write it. The input must meet the following
+   * criteria: - Be in a valid decimal format (integer with an optional '.'). - Not in scientific
+   * notation. - Fit within the precision and scale limits of decimal types.


Style: lines seem to have been accidentally wrapped.

rdblue · 2025-01-24T01:06:08Z

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

+   *
+   * @param value The numeric value to append.
+   */
+  protected void writeNumericInternal(long value) {


Why does this duplicate all of the serialization that is already implemented in PrimitiveWrapper?

I added writePrimitive() in ByteBufferWrapper which will call the serialization in PrimitiveWrapper.

rdblue · 2025-01-24T01:08:07Z

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

+        dataSize, fieldOffsetSize, offsetStart + numElements * fieldOffsetSize);
+  }
+
+  protected ByteBufferWrapper getBuffer() {


Style: Iceberg does not use get in accessor names and discourages its use in general. If there's a more helpful verb then it is generally better to use it. Otherwise we usually omit get.

rdblue · 2025-01-24T01:09:06Z

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

+     * @param value The numeric value to write.
+     * @param numBytes The number of bytes to write (e.g., 2 for short, 4 for int, 8 for long).
+     */
+    void writeLittleEndianUnsigned(long value, int numBytes) {


These utilities already exist. Why add them a second time? Are the existing ones incorrect?

I was trying to work on the resizable ByteBuffer and just implement the logic. But should be able to reuse. Let me update.

github-actions bot added the core label Dec 22, 2024

aihuaxu marked this pull request as ready for review December 22, 2024 20:11

aihuaxu force-pushed the variant-add-writer-impl branch 6 times, most recently from f8f8e8f to d806c66 Compare December 24, 2024 03:54

aihuaxu force-pushed the variant-add-writer-impl branch from d806c66 to 16210a0 Compare January 6, 2025 22:26

aihuaxu force-pushed the variant-add-writer-impl branch from 16210a0 to d4f1d0e Compare January 6, 2025 22:57

rdblue reviewed Jan 24, 2025

View reviewed changes

aihuaxu added 3 commits February 1, 2025 15:52

Add VariantBuilder

0e91759

Add tests

17661f7

Add array/object build interface

5ba965a

aihuaxu force-pushed the variant-add-writer-impl branch 2 times, most recently from 9cba102 to 86bf18b Compare February 2, 2025 03:59

Refactoring to use VariantUtil

b2290a0

aihuaxu force-pushed the variant-add-writer-impl branch 3 times, most recently from fa34686 to 3ad94d3 Compare February 3, 2025 19:10

aihuaxu requested a review from rdblue February 3, 2025 19:17

refactoring VariantBuilder

38e6f2f

aihuaxu force-pushed the variant-add-writer-impl branch from 3ad94d3 to 38e6f2f Compare February 3, 2025 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: add variant builder implementation #11857

Core: add variant builder implementation #11857

aihuaxu commented Dec 22, 2024 •

edited

Loading

aihuaxu commented Jan 6, 2025

rdblue Jan 24, 2025

aihuaxu Jan 31, 2025

aihuaxu Feb 3, 2025

rdblue Jan 24, 2025

aihuaxu Jan 31, 2025

rdblue Jan 24, 2025

rdblue Jan 24, 2025

aihuaxu Jan 31, 2025

rdblue Jan 24, 2025

aihuaxu Jan 31, 2025

rdblue Jan 24, 2025

aihuaxu Feb 1, 2025

rdblue Jan 24, 2025

rdblue Jan 24, 2025

aihuaxu Feb 2, 2025

rdblue Jan 24, 2025

rdblue Jan 24, 2025

aihuaxu Feb 1, 2025

Core: add variant builder implementation #11857

Are you sure you want to change the base?

Core: add variant builder implementation #11857

Conversation

aihuaxu commented Dec 22, 2024 • edited Loading

aihuaxu commented Jan 6, 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

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 commented Dec 22, 2024 •

edited

Loading