TimestampWriter isn't being used

[ashleycoxley]

When trying to write to a field with type Types.TimestampType.withoutZone(), I'm getting the exception
java.time.LocalDateTime cannot be cast to java.time.OffsetDateTime.

This appears to be due to a case match miss in the GenericParquetWriter - for all primitives of type TIMESTAMP_MICROS, the TimestamptzWriter is returned. I believe it should only be returned when shouldAdjustToUTC is true; and the TimestampWriter returned otherwise (similar to the implementation in GenericParquetReaders).

in GenericParquetWriter:

case TIMESTAMP_MICROS:
    return new TimestamptzWriter(desc);

in GenericParquetReader:

case TIMESTAMP_MICROS:
    TimestampType tsMicrosType = (TimestampType)expected;
    if (tsMicrosType.shouldAdjustToUTC()) {
        return new GenericParquetReaders.TimestamptzReader(desc);
    }

    return new GenericParquetReaders.TimestampReader(desc);

[rdblue]

You're right. The reason why this is a problem is that Parquet 1.10.1 doesn't have the shouldAdjustToUTC flag, and GenericParquetWriter uses only the Parquet schema to construct the writer. So there's no way to know whether the timestamp is with time zone or without time zone.

I think we could fix this by also passing in the Iceberg schema, which has the correct type. I think this would be fairly easy if we converted the builder to use TypeWithSchemaVisitor, which traverses a Parquet schema and an Iceberg schema at the same time.

[wangmiao1981]

@rdblue Just hit the same issue. In TypeToMessageType.java,
https://github.com/apache/incubator-iceberg/blob/master/parquet/src/main/java/org/apache/iceberg/parquet/TypeToMessageType.java#L82, calls https://github.com/apache/incubator-iceberg/blob/master/parquet/src/main/java/org/apache/iceberg/parquet/TypeToMessageType.java#L133
It can't store the shouldAdjustToUTC field. This is the reason it can't call the correct writer. So it fails in the if branch, https://github.com/apache/incubator-iceberg/blob/master/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L212

There are few other existing tests pass, which go to https://github.com/apache/incubator-iceberg/blob/master/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L235, which ignores the boolean flag and write it as Timestamp.withZone(). So it passes.

In Spark parquet writer, it actually throws an exception:

java.lang.UnsupportedOperationException: Spark does not support timestamp without time zone field

If I understand correctly, it may not be fixed by using TypeWithSchemaVisitor only. I will give it a try. I have made changes to throw similar exception as Spark does and comment out some tests. That was done before I found this opened issue.

If it is not an easy fix, I will submit the exception PR.

I also did a quick test on UUID and it also fails. I haven't debug that issue yet. I suspect similar issue of Logical Type support.

[rdblue]

I just took a look and it wasn't too bad to fix it by adding support for Parquet's new logical type annotations, which came out in 1.11.0 and store isAdjustedToUTC. Now, places that assumed the type was timestamptz can correctly distinguish between the two timestamp types. The PR is #891. We may need to add support in a few more places, I didn't have time to look very thoroughly.

[wangmiao1981]

+1 to add logical type. Will take a look the PR above. Just started reading source code. I may give a try.

Thanks!

[github-actions]

This issue has been automatically marked as stale because it has been open for 180 days with no activity. It will be closed in next 14 days if no further activity occurs. To permanently prevent this issue from being considered stale, add the label 'not-stale', but commenting on the issue is preferred when possible.

[github-actions]

This issue has been closed because it has not received any activity in the last 14 days since being marked as 'stale'