Add support for precision for TIMESTAMP in Postgresql type mapping #5124

losipiuk · 2020-09-09T20:44:19Z

Replaces #4570

mosabua · 2020-09-09T21:15:05Z

Please add type mapping docs for postgresql as part of this PR so it is clear to users.

findepi · 2020-09-10T08:02:25Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/PostgreSqlQueryRunner.java

+        return createPostgreSqlQueryRunner(server, extraProperties, connectorProperties, tables, 3);
+    }
+
+    public static DistributedQueryRunner createPostgreSqlQueryRunner(


I would rather drop this commit.
The added burden of an overload is not justified for arguably small benefit this brings.

findepi · 2020-09-10T08:03:31Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

@@ -1199,15 +1200,6 @@ public void testArrayTimestampWithTimeZone(boolean insertWithPresto)
        tests.execute(getQueryRunner(), sessionWithArrayAsArray(), dataSetup);
    }

-    @DataProvider
-    public Object[][] testTimestampWithTimeZoneDataProvider()


The cmt title is

Rename misleading testTimestampWithTimeZoneDataProvider

it's removal not "rename"

"misleading" is misleading. It was very nicely called "DataProvider", i do not find anything misleading in that and next time would call it the same.

Changed to "Use generic trueFalse data provider for timestamp tests"

findepi · 2020-09-10T08:09:59Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/StandardColumnMappings.java

+        long precision = timestampType.getPrecision();
+        checkArgument(precision <= MAX_SHORT_PRECISION, "Precision is out of range: %s", precision);
+        Instant instant = localDateTime.atZone(UTC).toInstant();
+        return instant.getEpochSecond() * MICROSECONDS_PER_SECOND + roundDiv(instant.getNano(), NANOSECONDS_PER_MICROSECOND);


if timestampType.precision < 6, you need to round/truncate here

@martint what is the correct way to do this?
there are some utility methods in io.prestosql.spi.type.Timestamps, but I do not see something that's immediately applicable.

findepi · 2020-09-10T08:10:54Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/StandardColumnMappings.java

    {
-        return Instant.ofEpochMilli(floorDiv(value, MICROSECONDS_PER_MILLISECOND)).atZone(UTC).toLocalDateTime();
+        int precision = timestampType.getPrecision();
+        checkArgument(precision <= MAX_SHORT_PRECISION && precision >= 0, "Precision is out of range: %s", precision);


You do not really need to validate this here. If someone provided epochMicros, it;'s all you need.

findepi · 2020-09-10T08:13:14Z

presto-base-jdbc/src/main/java/io/prestosql/plugin/jdbc/StandardColumnMappings.java

+        long epochSecond = floorDiv(epochMicros, MICROSECONDS_PER_SECOND);
+        int nanoFraction = floorMod(epochMicros, MICROSECONDS_PER_SECOND) * NANOSECONDS_PER_MICROSECOND;
+        Instant instant = Instant.ofEpochSecond(epochSecond, nanoFraction);
+        return LocalDateTime.ofInstant(instant, UTC);


we could unify & reuse with io.prestosql.spi.type.Timestamps#formatTimestamp(int precision, long epochMicros, int picosOfMicro)

We could but I am not sure if we could extend spi with random methods. We should keep it fairly stable. Also the spi methods cares about picosOfMicro which we do not care about here. With picosOfMicro you cannot create a method which just returns LocalDateTime, which we need here. And have a specific method version which does not take picosOfSeconds and returns LocalDateTime seems to specific to me, to be put in spi.

findepi · 2020-09-10T08:13:48Z

presto-mysql/src/main/java/io/prestosql/plugin/mysql/MySqlClient.java

@@ -262,7 +262,7 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
        }
        if (TIMESTAMP_MILLIS.equals(type)) {
            // TODO use `timestampWriteFunction`
-            return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp());
+            return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP));


Suggested change

return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP));

return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP_MILLIS));

findepi · 2020-09-10T08:18:15Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

-                .addRoundTrip(timestampDataType(), timeGapInKathmandu);
+        DataTypeTest tests = DataTypeTest.create(true);
+        for (int precision : List.of(3, 6)) {
+            // test all standard cases with precision 3 and 6 to make sure the long and short TIMESTAMP WITH TIME ZONE


"TIMESTAMP WITH TIME ZONE"

also, is 3 and 6 actually different cases?
i think in timestamp case, we go with the same code path

(ok to leave the test coverage, but if true, change the commnt)

Oh - you noticed it too - yeah - it same case. I dropped the coverage.

findepi · 2020-09-10T08:20:28Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

+        }
+    }
+
+    private DataType<List<LocalDateTime>> arrayOfTimestampDataType(int precision, boolean insertWithPresto)


this belong to Simplify flow in timestamp with timezone mapping tests commit?

no. This is specific to TIMESTAMP not TIMESTAMP w/TZ. So it belongs to last commit.

losipiuk · 2020-09-10T09:12:13Z

Please add type mapping docs for postgresql as part of this PR so it is clear to users.

This is a point change regarding one specific type and adding general type mapping documentation is substantial effort itself.
Added #5131 to track that.

Add support for mapping TIMESTAMP from Presto to PostgreSQL (and vice versa) for precisions 1 to 6. It covers all precisions currently supported by PostgreSQL.

losipiuk · 2020-09-10T09:37:39Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

+        }
+    }
+
+    private DataType<List<LocalDateTime>> arrayOfTimestampDataType(int precision, boolean insertWithPresto)


no. This is specific to TIMESTAMP not TIMESTAMP w/TZ. So it belongs to last commit.

losipiuk · 2020-09-10T09:38:19Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

-            dataType = arrayDataType(timestampDataType(), "timestamp[]");
-            dataSetup = postgresCreateAndInsert("tpch.test_array_timestamp");
+        DataTypeTest tests = DataTypeTest.create(true);
+        for (int precision : List.of(3, 6)) {


I will drop this for. As representation is same for 3 and 6 for TIMESTAMP

losipiuk · 2020-09-10T09:46:10Z

presto-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java

-                .addRoundTrip(timestampDataType(), timeGapInKathmandu);
+        DataTypeTest tests = DataTypeTest.create(true);
+        for (int precision : List.of(3, 6)) {
+            // test all standard cases with precision 3 and 6 to make sure the long and short TIMESTAMP WITH TIME ZONE


Oh - you noticed it too - yeah - it same case. I dropped the coverage.

kokosing added 4 commits September 9, 2020 22:41

Remove redundant cast

5a6a07e

Do not use raw parametrized type

65fd851

Remove not needed annotations

12c6906

Rename method paremeter to express its meaning

631f5aa

cla-bot bot added the cla-signed label Sep 9, 2020

losipiuk mentioned this pull request Sep 9, 2020

Support TIMESTAMP(6) in PostgreSql connector #4570

Closed

losipiuk requested review from findepi and kokosing September 9, 2020 20:44

mosabua added the needs-docs This pull request requires changes to the documentation label Sep 9, 2020

findepi approved these changes Sep 10, 2020

View reviewed changes

kokosing and others added 4 commits September 10, 2020 11:16

Use generic trueFalse data provider for timestamp tests

7b58796

Simplify flow in timestamp with timezone mapping tests

570a569

Inline addArrayTimestampTestIfSupported

91875da

Add support for precision for TIMESTAMP in PostgreSQL type mapping

e7fdfe3

Add support for mapping TIMESTAMP from Presto to PostgreSQL (and vice versa) for precisions 1 to 6. It covers all precisions currently supported by PostgreSQL.

losipiuk commented Sep 10, 2020

View reviewed changes

losipiuk force-pushed the lo/psql-timestamp-wo-tz-p branch from 305f378 to e7fdfe3 Compare September 10, 2020 10:04

losipiuk merged commit be2b97a into trinodb:master Sep 10, 2020

losipiuk mentioned this pull request Sep 11, 2020

Release notes for 342 #5111

Closed

9 tasks

martint added this to the 342 milestone Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for precision for TIMESTAMP in Postgresql type mapping #5124

Add support for precision for TIMESTAMP in Postgresql type mapping #5124

losipiuk commented Sep 9, 2020

mosabua commented Sep 9, 2020

findepi Sep 10, 2020

findepi Sep 10, 2020

losipiuk Sep 10, 2020

findepi Sep 10, 2020

findepi Sep 10, 2020

findepi Sep 10, 2020

losipiuk Sep 10, 2020

findepi Sep 10, 2020

findepi Sep 10, 2020

losipiuk Sep 10, 2020

findepi Sep 10, 2020

losipiuk Sep 10, 2020

losipiuk commented Sep 10, 2020

losipiuk Sep 10, 2020

losipiuk Sep 10, 2020

losipiuk Sep 10, 2020

	return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP));
	return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP_MILLIS));

Add support for precision for TIMESTAMP in Postgresql type mapping #5124

Add support for precision for TIMESTAMP in Postgresql type mapping #5124

Conversation

losipiuk commented Sep 9, 2020

mosabua commented Sep 9, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment