API, Core: Add default value APIs and Avro implementation #9502

wmoustafa · 2024-01-17T23:56:06Z

This PR adds default value APIs according to the default value spec, and implements it in the GenericAvroReader case. It uses a ConstantReader to fill in the default values of fields from their respective initialDefault() method, and rebases the implementation in #6004 to leverage new APIs introduced in #9366.

wmoustafa · 2024-06-14T17:40:08Z

@Fokko Could you please take a look since you helped with #9366? FYI @rdblue.

Fokko · 2024-06-17T18:45:48Z

core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java

@@ -155,6 +162,41 @@ public ValueReader<?> record(Type partner, Schema record, List<ValueReader<?>> f
      return recordReader(readPlan, avroSchemas.get(partner), record.getFullName());
    }

+    Object toGenericRecord(Type type, Object data) {
+      // Recursively convert data to GenericRecord if type is a StructType.
+      // TODO: Rewrite this as a visitor.


Converting this into a TypeUtil.SchemaVisitor would make sense

While I initially intended to implement it as a visitor, it turns out none of the existing visitor patterns (SchemaVisitor or SchemaWithPartnerVisitor) is a good fit for this use case since here we are co-traversing a schema and a data object (not just a schema or two schemas). I will clean up this code instead of using a visitor.

Fokko · 2024-06-17T18:48:25Z

core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java

@@ -155,6 +162,41 @@ public ValueReader<?> record(Type partner, Schema record, List<ValueReader<?>> f
      return recordReader(readPlan, avroSchemas.get(partner), record.getFullName());
    }

+    Object toGenericRecord(Type type, Object data) {


Nit

Suggested change

Object toGenericRecord(Type type, Object data) {

Object toGenericRecord(Type type, Object initialDefault) {

Fokko · 2024-06-17T18:56:33Z

core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java

+          genericRecord.put(
+              field.name(), toGenericRecord(field.type(), ((GenericRecord) data).get(index)));
+          index++;


Here we assume that the structs are in the same position. For example:

struct<a (1): int, b (2): string>

But they could be swapped:

struct<b (2): string, a (1): int>

Since these are Iceberg constructs, I would expect lookups by ID to be done here as well.

This will be converting actual data (some constant) coming from the schema. The data is represented in a JSON representation denoting the default value. Not sure if there will be a notion of field ID here. Further, since this is converting data from one representation to another, the target object is created from scratch and we are just reformatting the input object so not sure if field ID comparison applies here.

Fokko · 2024-06-17T19:11:19Z

core/src/test/java/org/apache/iceberg/avro/TestReadDefaultValues.java

+      Schema readerSchema =
+          new Schema(
+              required(999, "col1", Types.IntegerType.get()),
+              Types.NestedField.optional(1000, "col2", type, null, defaultValue, defaultValue));


Should we test the initialDefault and the writeDefault separately?

This PR does not implement writeDefault. Both implementation and test for writeDefault can be a separate line of work.

Where do we track all work to support default values?

Fokko · 2024-06-17T19:14:49Z

core/src/test/java/org/apache/iceberg/avro/TestReadDefaultValues.java

+          "{\"keys\": [1, 2], \"values\": [\"foo\", \"bar\"]}"
+        },
+        {
+          Types.StructType.of(


Do we also want to add some rainy-day scenarios, where a required/optional field is missing, out of order, etc

So I think this (test) code boils down to calling the new default value API in NestedField with both the field schema and its default value. I see the only thing that could go wrong here is the compatibility between the schema and the default value structure. If they are not compatible, then toGenericRecord will throw a cast exception (e.g., not being able to cast a map to a list, etc). Do we want to add tests that ensure some exception is thrown in this case (the incompatibility between data and type)?

Fokko · 2024-06-17T19:34:55Z

One more thing, we're also leaking V3 spec features into the codebase. Should we guard that with a flag?

wmoustafa · 2024-07-10T00:05:28Z

One more thing, we're also leaking V3 spec features into the codebase. Should we guard that with a flag?

It does not seem we can easily access format version in those reader classes. Since this feature is only accessible through a new API, do you think the guard could be on the client side in engine integrations?

rdblue · 2024-09-11T22:28:05Z

One more thing, we're also leaking V3 spec features into the codebase. Should we guard that with a flag?

For #9008, we added a check in TableMetadata.Builder to prevent v1 and v2 tables from using newer features. I think that we should update that check (Schema.checkCompatibility(Schema, int)) to include looking for defaults.

hussein-awala

LGTM

* Refactor to use a NestedField builder and remove nested defaults. * Remove unsupported test cases. * Apply spotless * Rename for clarity.

github-actions bot added API core labels Jan 17, 2024

wmoustafa force-pushed the default-value branch from 49a3b12 to 65c827c Compare April 2, 2024 17:07

This was referenced Jun 14, 2024

Why Iceberg do not support column with default value? #8527

Open

Support default value semantics in Iceberg #2039

Closed

Fokko self-requested a review June 14, 2024 19:52

Fokko reviewed Jun 17, 2024

View reviewed changes

Fokko added this to the Iceberg V3 Spec milestone Jul 31, 2024

nastra self-requested a review August 1, 2024 15:12

Fokko approved these changes Aug 15, 2024

View reviewed changes

Fokko mentioned this pull request Aug 16, 2024

Spec: Minor modifications for v3 #10948

Merged

wmoustafa changed the title ~~[Reference PR] [API + Avro] Add default value APIs and Avro implementation~~ [API + Avro] Add default value APIs and Avro implementation Aug 21, 2024

wmoustafa and others added 8 commits August 20, 2024 21:38

Add default value test cases

1746061

Add main default value methods

8a115c6

Add default value implementation for GenericAvroReader

09549ca

Convert Iceberg data to Avro data

2251e05

Fix ambiguous function call only taking place in Github checks

4bbd919

Refactor toGenericRecord

fa8d4ff

Make NestedField private

7d622c4

Rebase on main

a0792d5

wmoustafa force-pushed the default-value branch from 1720f5c to a0792d5 Compare August 21, 2024 04:47

rdblue mentioned this pull request Sep 3, 2024

Refactor to use a NestedField builder and remove nested defaults. wmoustafa/iceberg-1#1

Merged

manuzhang mentioned this pull request Sep 10, 2024

Support partial insert in merge into command #8199

Open

rdblue changed the title ~~[API + Avro] Add default value APIs and Avro implementation~~ API, Core: Add default value APIs and Avro implementation Sep 11, 2024

hussein-awala approved these changes Sep 19, 2024

View reviewed changes

rdblue and others added 3 commits October 1, 2024 16:24

Refactor to use a NestedField builder and remove nested defaults. (#1)

58957e7

* Refactor to use a NestedField builder and remove nested defaults. * Remove unsupported test cases. * Apply spotless * Rename for clarity.

Update some methods

7d66a52

Spotless apply

084d1b2

rdblue approved these changes Oct 4, 2024

View reviewed changes

rdblue merged commit 8190ce7 into apache:main Oct 4, 2024
50 checks passed

rdblue mentioned this pull request Oct 7, 2024

Core: Add internal Avro reader #11108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API, Core: Add default value APIs and Avro implementation #9502

API, Core: Add default value APIs and Avro implementation #9502

wmoustafa commented Jan 17, 2024 •

edited

Loading

wmoustafa commented Jun 14, 2024

Fokko Jun 17, 2024

wmoustafa Aug 14, 2024

Fokko Jun 17, 2024

Fokko Jun 17, 2024 •

edited

Loading

wmoustafa Jul 9, 2024

Fokko Jun 17, 2024

wmoustafa Jul 9, 2024

manuzhang Sep 10, 2024

Fokko Jun 17, 2024

wmoustafa Aug 14, 2024

Fokko commented Jun 17, 2024

wmoustafa commented Jul 10, 2024 •

edited

Loading

rdblue commented Sep 11, 2024

hussein-awala left a comment

	Object toGenericRecord(Type type, Object data) {
	Object toGenericRecord(Type type, Object initialDefault) {

API, Core: Add default value APIs and Avro implementation #9502

API, Core: Add default value APIs and Avro implementation #9502

Conversation

wmoustafa commented Jan 17, 2024 • edited Loading

wmoustafa commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko Jun 17, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko commented Jun 17, 2024

wmoustafa commented Jul 10, 2024 • edited Loading

rdblue commented Sep 11, 2024

hussein-awala left a comment

Choose a reason for hiding this comment

wmoustafa commented Jan 17, 2024 •

edited

Loading

Fokko Jun 17, 2024 •

edited

Loading

wmoustafa commented Jul 10, 2024 •

edited

Loading