Support TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC #307

findepi · 2019-02-25T08:52:54Z

dain · 2019-02-25T17:54:16Z

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java

        if (columnInfo.getColumnTypeName().equalsIgnoreCase("timestamp with time zone")) {
            try {
-                return new Timestamp(TIMESTAMP_WITH_TIME_ZONE_FORMATTER.parseMillis(String.valueOf(value)));
+                return TIMESTAMP_WITH_TIME_ZONE_FORMATTER.parse(String.valueOf(value), ZonedDateTime::from);


The JDBC 4.3 spec doesn't seem to define the correct behavior for timestamp with time zone like it does with timestamp (required to return java.sql.Timestamp). Do you know what other drivers do?

One interesting thing is the spec only ever talks about OffsetDateTime, likely because the SQL spec does not cover political time zones.

@dain good question. As discussed offline, this pertains to ResultSet#getObject(col) (i.e. without Class specifier).
From the database I have some working knowledge of, Oracle implements TIMESTAMP WITH TIME ZONE correctly, and probably Teradata as well.
I don't know yet what Oracle driver returns on getObject(col) (i would assume something compatible with what they picked when implementing JDBC 3...)

I think in Presto, getObject(col) should not return OffsetDateTime. This would be misleading, as the underlying value is not representable as OffsetDateTime without information loss.

PostgreSQL JDBC 42.2.5 returns java.sql.Timestamp by default and supports OffsetDateTime but not ZonedDateTime. The implementation is ... interesting:

Timestamp timestampValue = getTimestamp(columnIndex); ... // Postgres stores everything in UTC and does not keep original time zone OffsetDateTime offsetDateTime = OffsetDateTime.ofInstant(timestampValue.toInstant(), ZoneOffset.UTC);

The PostgreSQL docs:

For timestamp with time zone, the internally stored value is always in UTC

When a timestamp with time zone value is output, it is always converted from UTC to the current timezone zone, and displayed as local time in that zone

This seems to violate the SQL standard (and is different than Oracle), and the documentation doesn't mention it (and it mentions the standard in several other places).

Right, PostgreSQL'sTIMESTAMP WITH TIME ZONE is not standard-compliant. Their type represents a point in time (Instant in Java) instead of (point in time, zone) pair (ZonedDateTime in Java).
Thus, i don't see need to align with PostgreSQL driver behavior for this feature.

Agreed, returning ZonedDateTime seems correct for Presto.

electrum · 2019-02-26T01:43:23Z

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java

            }
        }

-        throw new IllegalArgumentException("Expected column to be a timestamp type but is " + columnInfo.getColumnTypeName());
+        throw new IllegalArgumentException("Expected column to be a timestamp with time zone type but is " + columnInfo.getColumnTypeName());


Let's put quotes or brackets around the timestamp with the zone type name

electrum · 2019-02-26T01:43:44Z

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java

@@ -331,16 +326,28 @@ private Timestamp getTimestamp(int columnIndex, DateTimeZone localTimeZone)
            }
        }

+        throw new IllegalArgumentException("Expected column to be a timestamp type but is " + columnInfo.getColumnTypeName());


Should this be SQLException? I see we use IAE already, but I think that's a bug

electrum · 2019-02-26T01:46:14Z

presto-jdbc/src/test/java/io/prestosql/jdbc/TestJdbcResultSet.java

            assertThrows(() -> rs.getDate(column));
            assertThrows(() -> rs.getTime(column));
-            assertEquals(rs.getTimestamp(column), Timestamp.valueOf(LocalDateTime.of(2018, 2, 13, 6, 14, 15, 227_000_000))); // TODO this should fail, or represent TIMESTAMP '2018-02-13 13:14:15.227'
+            assertThrows(() -> rs.getTimestamp(column)); // this could also return java.sql.Timestamp.valueOf(LocalDateTime.of(2018, 2, 13, 13, 14, 15, 227_000_000))


We should continue to allow calling getTimestamp() to not break applications.

Okay, and how do we deal with timestamps that are nor representable as java.sql.Timestamp (ie values when JVM zone had forward offset shift)?
just return some near value (as java.sql.Timestamp.valueOf(LocalDateTime) probably does)?

Yeah, I think using Timestamp.valueOf(LocalDateTime) is reasonable here.

That code path goes through the legacy Timestamp constructor which takes fields and calls into some sun.util.calendar code, so it's not clear what it does, but it seems fine to use the method since it exists in the JDK.

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java

guyco33 · 2019-05-27T13:03:52Z

I found this PR valuable.
Although the implementation of getObject is not backward compatible and it's not 100% consistent with respect to offset-literal-timestamps with JVM timezone, I found it much valuables due to the ability to perform timezone conversions with AT TIME ZONE.

Here is an example to illustrate this:
JVM timezone is set to "America/Bahia_Banderas" (UTC-5)

SQL text	Before	After
`SELECT current_timezone()`	America/Bahia_Banderas	America/Bahia_Banderas
`SELECT timestamp '2019-05-27 10:00:00'`	2019-05-27 10:00:00	2019-05-27 10:00:00
`SELECT timestamp '2019-05-27 10:00:00 +05:17'`	2019-05-26 23:43:00	2019-05-27 10:00:00
`SELECT timestamp '2019-05-27 10:00:00 UTC'`	2019-05-27 05:00:00	2019-05-27 10:00:00
`SELECT timestamp '2019-05-27 10:00:00' AT TIME ZONE 'UTC'`	2019-05-27 10:00:00	2019-05-27 15:00:00
`SELECT timestamp '2019-05-27 10:00:00 UTC' AT TIME ZONE 'UTC'`	2019-05-27 05:00:00	2019-05-27 10:00:00
`SELECT ts_col FROM tbl` (`tbl.ts_col` is a timestamp column)	2019-05-27 10:00:00	2019-05-27 10:00:00
`SELECT ts_col AT TIME ZONE 'UTC' FROM tbl`	2019-05-27 10:00:00	2019-05-27 15:00:00

@findepi @electrum @dain

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java

electrum · 2020-06-12T17:12:56Z

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java

+            ZonedDateTime zonedDateTime = getTimestampWithTimeZone(columnIndex);
+            verify(zonedDateTime != null);
+            // point in time
+            Timestamp timestamp = new Timestamp(zonedDateTime.toEpochSecond() * 1000);


SECONDS.toMillis(zonedDateTime.toEpochSecond())

But how is this different than

Timestamp.valueOf(zonedDateTime.toLocalDateTime())

This is different semantics.

Timestamp.valueOf(zonedDateTime.toLocalDateTime()) is like java.time.ZonedDateTime#withZoneSameLocal
current code is like java.time.ZonedDateTime#withZoneSameInstant

findepi · 2020-11-30T15:15:35Z

Based on #6152, other than that hopefully ready for review.

findepi · 2020-12-05T16:04:43Z

Depends on #6208, but otherwise ready for review.

cla-bot bot added the cla-signed label Feb 25, 2019

findepi mentioned this pull request Feb 25, 2019

TIMESTAMP behaviour does not match sql standard #37

Closed

26 tasks

dain reviewed Feb 25, 2019

View reviewed changes

electrum reviewed Feb 26, 2019

View reviewed changes

findepi added the roadmap:timestamp label Mar 18, 2019

dain changed the title ~~Represent TIMESTAMP W/TZ as ZonedDateTime in JDBC~~ [WIP] Represent TIMESTAMP W/TZ as ZonedDateTime in JDBC Apr 22, 2019

kokosing added WIP and removed WIP labels May 8, 2019

guyco33 reviewed May 24, 2019

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java Outdated Show resolved Hide resolved

guyco33 reviewed May 24, 2019

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java Outdated Show resolved Hide resolved

guyco33 reviewed May 24, 2019

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java Outdated Show resolved Hide resolved

findepi changed the title ~~[WIP] Represent TIMESTAMP W/TZ as ZonedDateTime in JDBC~~ [WIP] Represent TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC Feb 14, 2020

findepi changed the title ~~[WIP] Represent TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC~~ Represent TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC Feb 14, 2020

findepi force-pushed the jdbc-with-time-zone branch from e42a351 to ca9bdb2 Compare February 14, 2020 17:53

findepi marked this pull request as ready for review February 14, 2020 17:53

findepi removed the WIP label Feb 14, 2020

findepi force-pushed the jdbc-with-time-zone branch from ca9bdb2 to 8e0aa15 Compare February 14, 2020 21:23

findepi mentioned this pull request Feb 16, 2020

Support ZonedDateTime in QueryAssert trinodb/tempto#41

Merged

findepi force-pushed the jdbc-with-time-zone branch 2 times, most recently from 5aefe41 to d5e43f9 Compare February 17, 2020 07:59

findepi force-pushed the jdbc-with-time-zone branch from d5e43f9 to 65c2be3 Compare June 12, 2020 06:41

electrum reviewed Jun 12, 2020

View reviewed changes

presto-jdbc/src/main/java/io/prestosql/jdbc/PrestoResultSet.java Outdated Show resolved Hide resolved

electrum reviewed Jun 12, 2020

View reviewed changes

findepi mentioned this pull request Nov 10, 2020

Add Kafka INSERT smoke tests #5838

Closed

findepi removed the roadmap:timestamp label Nov 25, 2020

findepi force-pushed the jdbc-with-time-zone branch 2 times, most recently from e4fe71d to 406270f Compare November 30, 2020 13:29

findepi requested review from losipiuk and martint November 30, 2020 15:15

findepi mentioned this pull request Dec 4, 2020

Test TIMESTAMP WITH TIME ZONE calendar compatibility #6152

Merged

findepi changed the title ~~Represent TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC~~ Support TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC Dec 4, 2020

findepi force-pushed the jdbc-with-time-zone branch 2 times, most recently from e5b86e8 to 469690d Compare December 4, 2020 16:06

findepi added the jdbc Relates to Trino JDBC driver label Dec 4, 2020

This was referenced Dec 4, 2020

Consider representing date/time values in JDBC ResultSet.getObject using java.time classes #6209

Open

Convert array elements to proper representation #6208

Merged

findepi force-pushed the jdbc-with-time-zone branch from 469690d to 5545243 Compare December 4, 2020 22:14

findepi added the enhancement New feature or request label Dec 4, 2020

findepi force-pushed the jdbc-with-time-zone branch 2 times, most recently from 3b2568f to de66512 Compare December 5, 2020 07:52

losipiuk approved these changes Dec 8, 2020

View reviewed changes

findepi added 2 commits December 8, 2020 13:46

Separate TIMESTAMP [TZ] parsing and conversion

31da150

Support TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC

e558de2

findepi force-pushed the jdbc-with-time-zone branch from de66512 to e558de2 Compare December 8, 2020 12:46

findepi merged commit 2d92d05 into trinodb:master Dec 8, 2020

findepi deleted the jdbc-with-time-zone branch December 8, 2020 12:48

findepi mentioned this pull request Dec 8, 2020

Release notes for 348 #6100

Closed

9 tasks

findepi added this to the 348 milestone Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC #307

Support TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC #307

findepi commented Feb 25, 2019 •

edited

Loading

dain Feb 25, 2019

findepi Feb 25, 2019

electrum Feb 26, 2019 •

edited

Loading

electrum Feb 26, 2019 •

edited

Loading

findepi Feb 26, 2019

electrum Mar 6, 2019

electrum Feb 26, 2019

electrum Feb 26, 2019

electrum Feb 26, 2019

findepi Feb 26, 2019

electrum Mar 6, 2019

guyco33 commented May 27, 2019

electrum Jun 12, 2020

findepi Jun 12, 2020

findepi commented Nov 30, 2020

findepi commented Dec 5, 2020

Support TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC #307

Support TIMESTAMP WITH TIME ZONE as ZonedDateTime in JDBC #307

Conversation

findepi commented Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

electrum Feb 26, 2019 • 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

Choose a reason for hiding this comment

guyco33 commented May 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Nov 30, 2020

findepi commented Dec 5, 2020

findepi commented Feb 25, 2019 •

edited

Loading

electrum Feb 26, 2019 •

edited

Loading

electrum Feb 26, 2019 •

edited

Loading