Fix timestamp with timezone mapping in iceberg type converter #23534

auden-woolfson · 2024-08-27T21:59:44Z

Description

Fixes bug described in #23529

== RELEASE NOTES ==

Iceberg Connector Changes
* Add logic to iceberg type converter for timestamp with timezone :pr:`23534`

agrawalreetika

Can we add some tests with column types Timestamp with timezone?

tdcmeehan · 2024-08-28T13:21:36Z

+1. Let's add some end to end tests. Additionally, we may want to remove the validation added here, since I believe we should support this properly now: #22926

hantangwangd · 2024-08-28T16:56:45Z

Also add test cases involving Timestamp with timezone in filter conditions and partition columns, I'm a little concerned about the behavior in these scenarios.

auden-woolfson · 2024-08-28T23:52:08Z

Also add test cases involving Timestamp with timezone in filter conditions and partition columns, I'm a little concerned about the behavior in these scenarios.

Just to clarify, do you want the timestamp with timezone to be a part of the table that is being partitioned or the type of the partition column? Currently it is not supported as one of the types for partition columns.

hantangwangd · 2024-08-29T03:16:27Z

Just to clarify, do you want the timestamp with timezone to be a part of the table that is being partitioned or the type of the partition column? Currently it is not supported as one of the types for partition columns.

Yes, that's right. But I think it's better for us to first figure out how to handle it in these cases when we start to support it.

A very important question is, what format of long type data do we plan to actually store in data files for Timestamp with timezone? Presto has a special encoding for data with type of timestamp with timezone, which mix the time zone information with UTC values in millis. Meanwhile, Iceberg spec store the timestamp tz data as a UTC values in micros and do not retain the source time zone.

If we store the data following Iceberg spec, then we will lose the information of time zone; and if we store the data following Presto's format, then we may meet problems involving cross-engine compatibility.

cc: @tdcmeehan @agrawalreetika @ZacBlanco

tdcmeehan · 2024-08-29T12:42:57Z

@hantangwangd I don't see this as a choice, we must store the data according to the Iceberg spec, which means we'll lose the embedded time zone information. This is fine--semantically, it's the same thing, and the only thing that might be confusing is the user, when retrieving stored Iceberg timestamps, will see that the timezones have been adjusted to UTC. But the point in time values will remain the same, and this is merely a limitation of the Iceberg table format.

hantangwangd · 2024-08-29T16:21:09Z

@tdcmeehan Completely agree with your viewpoint.

That means we need to perform transformation logics for data with type of timestamp with timezone when writing/reading, parsing filter conditions, and handling partition values, besides doing the type conversion. It's not to say completing all these works all at once, but it can be divided into a series of PRs to complete.

ZacBlanco

Minor nits. One question about removing the verifyTypeSupported method

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergHiveMetadata.java

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergNativeMetadata.java

presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergTypes.java

agrawalreetika

Overall looks good to me.

Please add a document entry in https://prestodb.io/docs/current/connector/iceberg.html#type-mapping
Squash all the commits into 1 "Fix timestamp with timezone mapping in iceberg type converter"

agrawalreetika · 2024-09-14T03:56:00Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/TypeConverter.java

@@ -117,6 +117,10 @@ public static Type toPrestoType(org.apache.iceberg.types.Type type, TypeManager
            case TIME:
                return TimeType.TIME;
            case TIMESTAMP:
+                Types.TimestampType timestampType = (Types.TimestampType) type.asPrimitiveType();
+                if (timestampType.shouldAdjustToUTC()) {
+                    return TimestampWithTimeZoneType.TIMESTAMP_WITH_TIME_ZONE;


Add static import for this

hantangwangd · 2024-09-20T03:04:37Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergTypes.java

+        this.batchReaderEnabledQueryRunner = createIcebergQueryRunner(
+                ImmutableMap.of(),
+                ImmutableMap.of(),
+                ImmutableMap.of(PARQUET_BATCH_READ_OPTIMIZATION_ENABLED, "true"),


I think there is no need to refactor the IcebergQueryRunner.createIcebergQueryRunner(...), just set hive.parquet-batch-read-optimization-enabled to true in extraProperties would be ok.

Maybe there is another way, but adding this to extra properties actually causes the tests to break since it is an unused property there. This sets it as a configuration property, but it needs to be a session property. The session is actually passed to the distributed query runner builder in it's constructor, so we need some way to add properties to that session before building the runner. This is why I decided to make changes to IcebergQueryRunner. Please let me know if this clears things up, if you have another approach in mind here I would certainly be open to it!

Ooh, sorry for the overlooking that hive.parquet-batch-read-optimization-enabled belongs to the config in presto-hive-common. Since presto-main does not dependent on presto-hive-common, TestingPrestoServer does not inject HiveCommonModule either. So you are right currently we cannot set hive.parquet-batch-read-optimization-enabled in extraProperties here.

hantangwangd · 2024-09-20T03:16:08Z

...arquet/src/main/java/com/facebook/presto/parquet/reader/LongTimestampMicrosColumnReader.java

-            else {
-                type.writeLong(blockBuilder, utcMillis);
-            }
+            type.writeLong(blockBuilder, utcMillis);


The real long type data for timestamp with tz stored in parquet file is of presto's format? This may lead to many problems. As guided by @tdcmeehan, we should store the data in Iceberg format.

We may need to modify the writer as well for this, but I need to verify. I noticed that we were improperly packing the date in the previous logic which led to incorrect values being read.

For example the following set of queries produced bad results:

create table t(t TIMESTAMP WITH TIME ZONE); INSERT INTO t VALUES TIMESTAMP '1980-12-08 0:10:0 America/Los_Angeles'; presto:tpch> SELECT * FROM t; t ------------------------------- +46764-05-25 18:40:01.825 UTC

We may need to modify the writer as well for this.

Agree, the bad results in your example seems to be the problem caused by writing data with the incorrect format. So we need to customize a dedicated writer logic for timestamp with tz, which transform the long values encoded in presto's format to long values encoded in iceberg's format.

I made some changes that I thought would fix this, added the unpackMillisUtc method to a new TimestampWithTimezoneValueWriter that I added to parquet, but we are losing all timezone information in the unpacking bit shift. This theoretically shouldn't be a problem since we are supposed to be storing millisUtc in the first section of the bits, and the last 12 are reserved for whatever timezone we want to display to the user. However this is clearly not the case, since the filter operations are failing, the program expects the millis UTC to be transformed based on the timezone info, to get an actual millis UTC.

This is some pretty misleading variable naming and I think it leaves us with two options...

leave the system the same and build methods on top of it that apply the timezone part of the ts with tz to the millis part to get a correct instant (and change the variable names)

change the way ts with tz is read so at the time of unpacking we already have the millis UTC correct.

Sorry if this is redundant, I couldn't find documentation about how iceberg stores ts w tz under the hood.

Referring to https://iceberg.apache.org/spec/#primitive-types, Iceberg defines timestamp tz as follows:

Timestamp, microsecond precision, with timezone Timestamp values with time zone represent a point in time: values are stored as UTC and do not retain a source time zone (2017-11-16 17:10:34 PST is stored/retrieved as 2017-11-17 01:10:34 UTC and these values are considered identical).

So it just contains UTC values in micros and do not retain the source time zone.

By the way, did the filtering operation fail because we forgot to do the same convert for timestamp with tz in ExpressionConverter.getIcebergLiteralValue(...)?

Not sure about the filtering operation. I had to add TimestampWithTimezoneType to the cases in expression converter but so far I have just treated it like the other date time types that are included. I tried adding unpacking to the expression converter for ts w tz and now we have the regular tests passing, but failing with the batch reader enabled. Looking into this now

Edit: fixed by adding the long value conversion (packing) to the plain batch values decoder

auden-woolfson · 2024-09-24T23:46:12Z

...to-parquet/src/main/java/com/facebook/presto/parquet/batchreader/decoders/ValuesDecoder.java

@@ -60,6 +60,9 @@ interface Int64TimeAndTimestampMicrosValuesDecoder
        void readNext(long[] values, int offset, int length)
                throws IOException;

+        void readNextWithTimezone(long[] values, int offset, int length)
+                throws IOException;
+


This is necessary because the decoder is accesses the individual raw values from parquet, not the batch reader. The batch reader has the column descriptor (metadata) which should tell it whether or not to pack the value with timezone, like is done in the regular column reader. The actual packing should be done within the decoder, so there needs to be a way for decoders to dynamically switch between with and without timezone mode.

The approach I took here is just to copy the readNext method from each implementation and add packDateTimewithZone. A stateful approach (instance variable bool withTimestamp or something) would probably be more efficient for future development and code execution but for now I am seeing if this implementation works.

ZacBlanco · 2024-10-21T13:53:51Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/TestIcebergTypes.java

+    public Object[][] createTestTimestampWithTimezoneData()
+    {
+        return new Object[][] {
+                {getQueryRunner()},


Wondering if we need a whole separate test for this. Can't we just create a dataProvider which passes in true/false values and lets us construct a valid session in the beginning of the test method? Then you can pass the session to all of the execute/assertQuery methods?

Can you elaborate on this please? I'm not sure what you are referring to as the separate test. I can have the data provider pass in one true and one false value and add a condition inside the test function itself, is that what you are asking for here? If so what purpose would that serve? Thanks

ZacBlanco · 2024-10-21T13:57:09Z

...to-parquet/src/main/java/com/facebook/presto/parquet/batchreader/decoders/ValuesDecoder.java

+
+        boolean isWithTimezone();
+
+        void setWithTimezone(boolean withTimezone);


The API for this is an anti-pattern. We should aim for the reader to be immutable. I think the implementors of this ValuesDecoder should have a constructor where this is set instead.

ZacBlanco · 2024-10-21T14:00:16Z

.../parquet/batchreader/decoders/rle/Int64TimeAndTimestampMicrosRLEDictionaryValuesDecoder.java

@@ -69,7 +91,13 @@ public void readNext(long[] values, int offset, int length)
                    final LongDictionary localDictionary = dictionary;
                    for (int srcIndex = currentBuffer.length - currentCount; destinationIndex < endIndex; srcIndex++) {
                        long dictionaryValue = localDictionary.decodeToLong(localBuffer[srcIndex]);
-                        values[destinationIndex++] = MICROSECONDS.toMillis(dictionaryValue);
+                        long millisValue = MICROSECONDS.toMillis(dictionaryValue);
+                        if (isWithTimezone()) {


Rather than having a branch statement inside the hot loop for a lot of these readers, I think we should set the "reading function" in the constructor of the reader as the reading behavior shouldn't change. Do you see a performance impact when this conditional is introduced?

ZacBlanco · 2024-10-21T14:00:51Z

...in/java/com/facebook/presto/parquet/writer/valuewriter/TimestampWithTimezoneValueWriter.java

+        for (int i = 0; i < block.getPositionCount(); i++) {
+            if (!block.isNull(i)) {
+                long value = unpackMillisUtc(type.getLong(block, i));
+                long scaledValue = writeMicroseconds ? MILLISECONDS.toMicros(value) : value;


same comment with avoiding branching inside hot loop

auden-woolfson added bug iceberg Apache Iceberg related labels Aug 27, 2024

auden-woolfson self-assigned this Aug 27, 2024

auden-woolfson requested review from hantangwangd, ZacBlanco and a team as code owners August 27, 2024 21:59

auden-woolfson requested a review from presto-oss August 27, 2024 21:59

auden-woolfson force-pushed the add_timestamptz_mapping_to_iceberg_connector branch from 8e1716e to 38919d7 Compare August 27, 2024 22:01

auden-woolfson linked an issue Aug 27, 2024 that may be closed by this pull request

Iceberg timestamptz should map to Presto TIMESTAMP WITH TIME ZONE type #23529

Open

agrawalreetika reviewed Aug 28, 2024

View reviewed changes

tdcmeehan self-assigned this Aug 28, 2024

auden-woolfson requested a review from shangxinli as a code owner August 28, 2024 21:27

ZacBlanco requested changes Sep 5, 2024

View reviewed changes

auden-woolfson force-pushed the add_timestamptz_mapping_to_iceberg_connector branch 2 times, most recently from a562f45 to d877bcc Compare September 13, 2024 21:50

agrawalreetika requested changes Sep 14, 2024

View reviewed changes

hantangwangd requested changes Sep 20, 2024

View reviewed changes

auden-woolfson force-pushed the add_timestamptz_mapping_to_iceberg_connector branch from 69a4878 to 361d23a Compare September 24, 2024 19:54

auden-woolfson commented Sep 24, 2024

View reviewed changes

auden-woolfson force-pushed the add_timestamptz_mapping_to_iceberg_connector branch from 79d53ce to 607edf0 Compare September 25, 2024 17:59

auden-woolfson force-pushed the add_timestamptz_mapping_to_iceberg_connector branch from a381447 to c7deb43 Compare October 18, 2024 16:08

ZacBlanco requested changes Oct 21, 2024

View reviewed changes

auden-woolfson requested a review from steveburnett as a code owner October 21, 2024 20:35

auden-woolfson added 29 commits November 25, 2024 11:30

update

053f180

fix iceberg distributed smoke test base

233651d

outline new timestamp with tz parquet writer logic

9ddb17c

add unpacking to expression converter

ac40cc7

pack value with zone utc in plain values decoder for batch reader

95ca6cd

add readNextWithTimezone method to decoder interface/implementations

14e8909

pull timestamp batch readers out of generated sources

a20d4ec

add timezone logic to batch readers, tests passing

f3cd0fe

checkstyle

989a048

use stateful approach to decode with timestamp logic

3765ce2

checkstyle

61aa9ee

add withTimezone to constructors

9d82cf8

make constructor used setter for withTimezone

fe8d2ee

remove setWithTimezone method

7ec5853

set packing function in constructor of RLE class

0f4cb2c

remove unused import

3fd542e

add functional interface to other decoder implementations

6ec4b13

checkstyle

392ddbc

eliminate branching in hot loop for value writer

ac53703

add timestamp with timezone type mapping to iceberg connector docs

ea8d84a

set value to pack function in packed values decoder

04788ff

clean value writer code

0093fcc

rm isWithTimezone method from interface

d9f223a

added withTimezone arg to all decoders

6e859be

fix decoder casts

adde43f

saved

d7473ec

changes

2b70909

remove debug logs in nested batch reader, confirmed not the problem

b6c5cce

remove use of packing function from plain decoder

e2f6634

auden-woolfson force-pushed the add_timestamptz_mapping_to_iceberg_connector branch from 9e8764b to e2f6634 Compare November 25, 2024 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix timestamp with timezone mapping in iceberg type converter #23534

Fix timestamp with timezone mapping in iceberg type converter #23534

auden-woolfson commented Aug 27, 2024 •

edited

Loading

agrawalreetika left a comment

tdcmeehan commented Aug 28, 2024

hantangwangd commented Aug 28, 2024

auden-woolfson commented Aug 28, 2024

hantangwangd commented Aug 29, 2024

tdcmeehan commented Aug 29, 2024

hantangwangd commented Aug 29, 2024

ZacBlanco left a comment

agrawalreetika left a comment

agrawalreetika Sep 14, 2024

hantangwangd Sep 20, 2024

auden-woolfson Sep 20, 2024

hantangwangd Sep 23, 2024

hantangwangd Sep 20, 2024

ZacBlanco Sep 20, 2024

hantangwangd Sep 20, 2024

auden-woolfson Sep 20, 2024

hantangwangd Sep 23, 2024 •

edited

Loading

auden-woolfson Sep 23, 2024 •

edited

Loading

auden-woolfson Sep 24, 2024

ZacBlanco Oct 21, 2024

auden-woolfson Oct 21, 2024

ZacBlanco Oct 21, 2024

ZacBlanco Oct 21, 2024

ZacBlanco Oct 21, 2024


		boolean isWithTimezone();

		void setWithTimezone(boolean withTimezone);

Fix timestamp with timezone mapping in iceberg type converter #23534

Are you sure you want to change the base?

Fix timestamp with timezone mapping in iceberg type converter #23534

Conversation

auden-woolfson commented Aug 27, 2024 • edited Loading

Description

agrawalreetika left a comment

Choose a reason for hiding this comment

tdcmeehan commented Aug 28, 2024

hantangwangd commented Aug 28, 2024

auden-woolfson commented Aug 28, 2024

hantangwangd commented Aug 29, 2024

tdcmeehan commented Aug 29, 2024

hantangwangd commented Aug 29, 2024

ZacBlanco left a comment

Choose a reason for hiding this comment

agrawalreetika 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

hantangwangd Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

auden-woolfson Sep 23, 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

auden-woolfson commented Aug 27, 2024 •

edited

Loading

hantangwangd Sep 23, 2024 •

edited

Loading

auden-woolfson Sep 23, 2024 •

edited

Loading