Data: Fix Parquet and Avro defaults date/time representation #11811

rdblue · 2024-12-18T16:12:55Z

This updates the Avro and Parquet readers for Spark and Iceberg's generic data model to produce the expected date/time classes. It also adds a test for primitive type defaults.

rdblue · 2024-12-18T16:16:23Z

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

-        readPlan.add(Pair.of(pos, ValueReaders.constant(field.initialDefault())));
+        readPlan.add(
+            Pair.of(
+                pos, ValueReaders.constant(convert.apply(field.type(), field.initialDefault()))));


This raises a question: should we also convert the constants passed by idToConstant?

I think we should but not yet. The problem is that this conversion probably can't be done twice because of assumptions in the conversion (like casting strings to UTF8String in Spark) so it is worth a separate PR to refactor all of the conversions to be done in one place. That will also be different for each file format so it could get messy.

Let's do that in a separate PR indeed 👍

rdblue · 2024-12-18T16:18:40Z

data/src/test/java/org/apache/iceberg/data/DataTestHelpers.java

+        assertEquals(
+            field.type(),
+            GenericDataUtil.internalToGeneric(field.type(), field.initialDefault()),
+            actual.getField(field.name()));


This was the test bug. Rather than asserting that the value was passed back unchanged, it should have delegated to assertEquals that checks the representation of the value.

The expected value (default) also needs to be converted to generic because the generic data model tests use the generic data model as the source representation.

rdblue · 2024-12-18T18:55:37Z

core/src/main/java/org/apache/iceberg/data/avro/PlannedDataReader.java

@@ -97,7 +98,8 @@ public ValueReader<?> record(Type partner, Schema record, List<ValueReader<?>> f

      Types.StructType expected = partner.asStructType();
      List<Pair<Integer, ValueReader<?>>> readPlan =
-          ValueReaders.buildReadPlan(expected, record, fieldReaders, idToConstant);
+          ValueReaders.buildReadPlan(
+              expected, record, fieldReaders, idToConstant, GenericDataUtil::internalToGeneric);


This change is only needed for the generic data model and not for the internal data model because defaults are already using the correct classes for internal.

rdblue · 2024-12-18T18:56:45Z

data/src/test/java/org/apache/iceberg/data/DataTest.java


 public abstract class DataTest {

  protected abstract void writeAndValidate(Schema schema) throws IOException;

+  protected void writeAndValidate(Schema writeSchema, Schema expectedSchema) throws IOException {


To deduplicate code, I moved the new default test cases to the DataTest and AvroDataTest classes and disable them with assumptions where defaults aren't needed or supported.

rdblue · 2024-12-18T18:58:35Z

spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/data/TestSparkAvroReader.java

+  }
+
+  @Override
+  protected void writeAndValidate(Schema writeSchema, Schema expectedSchema) throws IOException {


Adding default tests to the Avro read path as well.

Fokko

Few nits around unboxing, but looks good 👍 Thanks for cleaning up the tests

Fokko · 2024-12-18T19:47:24Z

core/src/main/java/org/apache/iceberg/data/GenericDataUtil.java

+
+    switch (type.typeId()) {
+      case DATE:
+        return DateTimeUtil.dateFromDays((Integer) value);


Suggested change

return DateTimeUtil.dateFromDays((Integer) value);

return DateTimeUtil.dateFromDays((int) value);

Value is an Object, so I cast to the object rather than the primitive. Looks like you're right that we can cast directly and unbox at the same time based on the method signature... but I typically don't do that for style reasons. If you cast to the primitive, then there's a risk of introducing an unnecessary failure if the value is null and the receiving function accepts an Integer that can be null. I think casting to the object version is better to avoid those problems.

My reasoning is similar, but the other way around:

As you mentioned, the method accepts an int:
https://github.com/apache/iceberg/blob/91a1505d09cebcd1d088ac53cd42732c343883de/api/src/main/java/org/ap
ache/iceberg/util/DateTimeUtil.java#L49-L51

Casting to an Integer gives the impression to me that a null is acceptable here, while it is not. I'm not very strong on this one, let's pull in @nastra as a tiebreaker :)

I agree with Ryan's reasoning. Casting to int will throw a NPE because it will first cast it to Integer and then throw the NPE when doing Integer.intValue(). That being said, it's safer to always cast to the object type instead of the primitive in case the value can be null

Fokko · 2024-12-18T19:47:32Z

core/src/main/java/org/apache/iceberg/data/GenericDataUtil.java

+      case DATE:
+        return DateTimeUtil.dateFromDays((Integer) value);
+      case TIME:
+        return DateTimeUtil.timeFromMicros((Long) value);


Suggested change

return DateTimeUtil.timeFromMicros((Long) value);

return DateTimeUtil.timeFromMicros((long) value);

Fokko · 2024-12-18T19:47:42Z

core/src/main/java/org/apache/iceberg/data/GenericDataUtil.java

+        return DateTimeUtil.timeFromMicros((Long) value);
+      case TIMESTAMP:
+        if (((Types.TimestampType) type).shouldAdjustToUTC()) {
+          return DateTimeUtil.timestamptzFromMicros((Long) value);


Suggested change

return DateTimeUtil.timestamptzFromMicros((Long) value);

return DateTimeUtil.timestamptzFromMicros((long) value);

Fokko · 2024-12-18T19:47:49Z

core/src/main/java/org/apache/iceberg/data/GenericDataUtil.java

+        if (((Types.TimestampType) type).shouldAdjustToUTC()) {
+          return DateTimeUtil.timestamptzFromMicros((Long) value);
+        } else {
+          return DateTimeUtil.timestampFromMicros((Long) value);


Suggested change

return DateTimeUtil.timestampFromMicros((Long) value);

return DateTimeUtil.timestampFromMicros((long) value);

Fokko · 2024-12-18T20:01:15Z

spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkUtil.java

    if (value == null) {
      return null;
    }

    switch (type.typeId()) {
      case DECIMAL:
        return Decimal.apply((BigDecimal) value);
+      case UUID:


Good catch:

Fokko · 2024-12-18T20:06:53Z

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

-        readPlan.add(Pair.of(pos, ValueReaders.constant(field.initialDefault())));
+        readPlan.add(
+            Pair.of(
+                pos, ValueReaders.constant(convert.apply(field.type(), field.initialDefault()))));


Let's do that in a separate PR indeed 👍

rdblue · 2024-12-18T21:38:50Z

Thanks for reviewing, @Fokko!

github-actions bot added parquet core data labels Dec 18, 2024

rdblue mentioned this pull request Dec 18, 2024

Spark 3.5: Support default values in Parquet reader #11803

Merged

rdblue commented Dec 18, 2024

View reviewed changes

rdblue added 2 commits December 18, 2024 10:31

Data: Fix Parquet and Avro generic date/time representation.

fd543aa

Refactor tests and apply the same fixes to Spark.

3f90aa8

rdblue force-pushed the fix-generic-default-classes branch from 9ad4789 to 3f90aa8 Compare December 18, 2024 18:53

github-actions bot added the spark label Dec 18, 2024

rdblue changed the title ~~Data: Fix Parquet and Avro generic date/time representation~~ Data: Fix Parquet and Avro defaults date/time representation Dec 18, 2024

rdblue commented Dec 18, 2024

View reviewed changes

Fokko approved these changes Dec 18, 2024

View reviewed changes

rdblue merged commit d0effc6 into apache:main Dec 18, 2024
49 checks passed

rdblue mentioned this pull request Dec 19, 2024

Spark: Test reading default values in Spark #11832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data: Fix Parquet and Avro defaults date/time representation #11811

Data: Fix Parquet and Avro defaults date/time representation #11811

rdblue commented Dec 18, 2024 •

edited

Loading

rdblue Dec 18, 2024

Fokko Dec 18, 2024

rdblue Dec 18, 2024

rdblue Dec 18, 2024

rdblue Dec 18, 2024

rdblue Dec 18, 2024

Fokko left a comment

Fokko Dec 18, 2024

rdblue Dec 18, 2024

Fokko Dec 19, 2024

nastra Dec 19, 2024

Fokko Dec 18, 2024

Fokko Dec 18, 2024

Fokko Dec 18, 2024

Fokko Dec 18, 2024

Fokko Dec 18, 2024

rdblue commented Dec 18, 2024

	return DateTimeUtil.dateFromDays((Integer) value);
	return DateTimeUtil.dateFromDays((int) value);

	return DateTimeUtil.timeFromMicros((Long) value);
	return DateTimeUtil.timeFromMicros((long) value);

	return DateTimeUtil.timestamptzFromMicros((Long) value);
	return DateTimeUtil.timestamptzFromMicros((long) value);

	return DateTimeUtil.timestampFromMicros((Long) value);
	return DateTimeUtil.timestampFromMicros((long) value);

Data: Fix Parquet and Avro defaults date/time representation #11811

Data: Fix Parquet and Avro defaults date/time representation #11811

Conversation

rdblue commented Dec 18, 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

Fokko 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

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 Dec 18, 2024

rdblue commented Dec 18, 2024 •

edited

Loading