Add support for precision for TIMESTAMP W/TZ in Postgresql type mapping #5105
Conversation
|
Relates to #4570 |
| @@ -434,8 +442,15 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type) | |||
| if (TIMESTAMP_MILLIS.equals(type)) { | |||
| return WriteMapping.longMapping("timestamp", timestampWriteFunction()); | |||
| } | |||
| if (TIMESTAMP_WITH_TIME_ZONE.equals(type)) { | |||
| return WriteMapping.longMapping("timestamptz", timestampWithTimeZoneWriteFunction()); | |||
| if (type instanceof TimestampWithTimeZoneType && ((TimestampWithTimeZoneType) type).getPrecision() <= 6) { | |||
There was a problem hiding this comment.
Postgres currently supports p <=6 (according to docs), so we do not have behavioral regression here.
Worth mentioning in the commit message that we are leveraging this fact.
There was a problem hiding this comment.
Until 9 we would be fine. It would harder with 10,11,12 as I am not sure if that is compatible with JDBC. At least not via OffsetDateTime PSQL uses.
The ((TimestampWithTimeZoneType) type).getPrecision() <= 6 condition is merly a checkstate. I just did not want to have it explicitly in if (type instanceof TimestampWithTimeZoneType) {, so standard unsupported type handling logic triggers if for some magical reason we get timestamp with precision >6 (e.g with some future version of PSQL)
| TIMESTAMP_WITH_TIME_ZONE, | ||
| timeStampWithTimeZoneReadFunction(), | ||
| timestampWithTimeZoneWriteFunction()); | ||
| checkArgument(precision <= 6, "unsupported precision value"); |
There was a problem hiding this comment.
Add
// PostgreSQL currently supports precision up to microseconds
to argument that throwing is actually OK think to do
| (statement, index, value) -> { | ||
| // PostgreSQL does not store zone information in "timestamp with time zone" data type | ||
| long epochSeconds = value.getEpochMillis() / MILLISECONDS_PER_SECOND; | ||
| long nanosOfSecond = value.getEpochMillis() % MICROSECONDS_PER_MILLISECOND * NANOSECONDS_PER_MILLISECOND + value.getPicosOfMilli() / PICOSECONDS_PER_NANOSECOND; |
There was a problem hiding this comment.
MICROSECONDS_PER_MILLISECOND -> MILLISECONDS_PER_SECOND
@martint is the % and / right here? or floorDiv instead?
do we have helper methods somewhere so that we do not have to ask the above question ever again?
| else { | ||
| LongTimestampWithTimeZone value = (LongTimestampWithTimeZone) prestoNative; | ||
| long epochSeconds = value.getEpochMillis() / MILLISECONDS_PER_SECOND; | ||
| long nanosOfSecond = value.getEpochMillis() % MICROSECONDS_PER_MILLISECOND * NANOSECONDS_PER_MILLISECOND |
There was a problem hiding this comment.
MICROSECONDS_PER_MILLISECOND -> ..
| @@ -1141,16 +1141,16 @@ public void testTimestampWithTimeZone(boolean insertWithPresto) | |||
| } | |||
|
|
|||
| @Test(dataProvider = "testTimestampWithTimeZoneDataProvider") | |||
| public void testArrayTimestampWithTimeZone(boolean insertWithPresto) | |||
| public void testArrayTimestampWithTimeZone(boolean insertWithPresto, int precision) | |||
There was a problem hiding this comment.
Let's also add test what happens if we try create table with timestamp(7).
| private static ZonedDateTime roundToPrecision(ZonedDateTime zonedDateTime, int precision) | ||
| { | ||
| int divisor = (int) Math.pow(10, 9 - precision); | ||
| return zonedDateTime.withNano(zonedDateTime.getNano() / divisor * divisor); |
There was a problem hiding this comment.
Method is called "round to precision", but this truncates.
There was a problem hiding this comment.
Yeah - previous version used to round :)
losipiuk
force-pushed
the
lo/psql-timestamp-p
branch
from
7fa8471 to
2394c13
Compare
|
AC |
Add support for mapping TIMESTAMP WITH TIME ZONE from Presto to PostgreSQL (and vice versa) for precisions 1 to 6. It covers all precisions currently supported by PostgreSQL.
losipiuk
force-pushed
the
lo/psql-timestamp-p
branch
from
2394c13 to
e12f1ba
Compare
|
@findepi I reworked testing towards what we discussed |
No description provided.