Support TIMESTAMPS with precisions higher than 3 in MySQL #8060

guyco33 · 2021-05-24T18:43:41Z

Solves #6910
Fixes #7413

guyco33 · 2021-05-26T15:13:47Z

@findepi, PR ready to review

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

findepi · 2021-05-31T13:55:56Z

cc @hashhar @losipiuk @wendigo

hashhar

Sorry for the delay @guyco33. The impl looks good % Piotr's comments.

The tests can use more cases. Most of the tested values have trailing 0s - you can copy the values from PostgreSQL tests.

Some values don't round-trip and it's a known issue (#7201 (comment)) - don't "fix" those test values - leave them be. They should be fixed only after the DST handling gets fixed.

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

hashhar

Some comments. Sorry for the delay @guyco33.

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

findepi

good job!

plugin/trino-mysql/pom.xml

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

guyco33 · 2021-06-16T16:27:51Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

+     * @see #testTimestampFromTrino
+     */
+    @Test
+    public void testTimestampCoercion()


copied from PostgreSQL

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlJdbcConfig.java

hashhar

Looks good, some comments.

pom.xml

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlJdbcConfig.java

plugin/trino-raptor-legacy/src/main/java/io/trino/plugin/raptor/legacy/util/DatabaseUtil.java

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

guyco33 · 2021-06-17T15:08:04Z

@hashhar, in ece6e6e I removed duplicate tests, reorder them and change 0s values to contain higher precision

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

findepi · 2021-06-22T10:29:16Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

@@ -605,88 +594,75 @@ public void testTimestampFromMySql(ZoneId sessionZone)
                // before epoch (MySQL's timestamp type doesn't support values <= epoch)
                //.addRoundTrip("timestamp(3)", "TIMESTAMP '1958-01-01 13:18:03.123'", createTimestampType(3), "TIMESTAMP '1958-01-01 13:18:03.000'")


is this comment outdated?
i see test values like 1969-12-31 23:59:59.1230001, which is before epoch

Mysql timestamp type is not supporting negative epoch

i understand now:

we're testing negative epoch when we insert with MySQL datetime, eg
// negative epoch .addRoundTrip("datetime(6)", "TIMESTAMP '1969-12-31 23:59:59.999995'", createTimestampType(6), "TIMESTAMP '1969-12-31 23:59:59.999995'")

we cannot test roundtrip when inserting with MySQL timestamp, because it doesn't support negative epoch

we should be able to test roundtrip when CT/CTAS with Trino, because Trino timestamp is mapped to datetime

is there a test for last bullet?

is there a test for last bullet?

Yes we have it in testTimestampCoercion L639 and L676:L679 and also in testTimestampFromTrino L601 and L609

findepi · 2021-06-22T10:29:53Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

                // epoch (MySQL's timestamp type doesn't support values <= epoch)
-                //.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:00:00.000'", createTimestampType(3), "TIMESTAMP '1970-01-01 00:00:00.000'")


is this comment outdated?
i see test values like 1969-12-31 23:59:59.1230001, which is before epoch

It's not outdated. Mysql timestamp type doesn't support negative epoch. Maybe we can completely remove it since it's not a valid Mysql input value.

findepi · 2021-06-22T10:30:04Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

-                //.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:00:00.000'", createTimestampType(3), "TIMESTAMP '1970-01-01 00:00:00.000'")
-                .addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:13:42.000'", TIMESTAMP_MILLIS, "TIMESTAMP '1970-01-01 00:13:42.000'")
-                .addRoundTrip("timestamp(3)", "TIMESTAMP '2018-04-01 02:13:55.123'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-04-01 02:13:55.123'")
+                //.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:00:00.000'", createTimestampType(3), "TIMESTAMP '1970-01-01 01:00:00.000'")


undesired change on this line (hour 00 -> 01). please revert.

1970-01-01 00:00:00 is not a valid value for Mysql timestamp type.
It has a range of 1970-01-01 00:00:01' UTC to '2038-01-19 03:14:07' UTC
https://dev.mysql.com/doc/refman/8.0/en/datetime.html#:~:text=MySQL%20retrieves%20and%20displays%20DATETIME,%3A14%3A07'%20UTC.
I will remove it from tests as all other comments and unsupported Mysql input values

hashhar

LGTM % comments.

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java

hashhar

LGTM.

guyco33 · 2021-06-27T20:17:47Z

@findepi It's ready to merge from my side

hashhar · 2021-06-28T15:42:12Z

Thanks a lot for working on this @guyco33 . Merged.

cla-bot bot added the cla-signed label May 24, 2021

martint requested a review from findepi May 24, 2021 18:44

guyco33 force-pushed the fix_6910 branch from 70af2f6 to b646516 Compare May 24, 2021 19:33

findepi reviewed May 31, 2021

View reviewed changes

hashhar reviewed May 31, 2021

View reviewed changes

guyco33 force-pushed the fix_6910 branch from 5ff6c19 to df1ffda Compare June 14, 2021 13:05

hashhar reviewed Jun 14, 2021

View reviewed changes

hashhar reviewed Jun 15, 2021

View reviewed changes

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java Outdated Show resolved Hide resolved

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java Outdated Show resolved Hide resolved

findepi reviewed Jun 16, 2021

View reviewed changes

guyco33 force-pushed the fix_6910 branch from a7b5707 to 80da1d3 Compare June 16, 2021 16:25

guyco33 commented Jun 16, 2021

View reviewed changes

guyco33 force-pushed the fix_6910 branch from 80da1d3 to 05ee2ea Compare June 16, 2021 17:43

findepi reviewed Jun 16, 2021

View reviewed changes

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlJdbcConfig.java Outdated Show resolved Hide resolved

guyco33 force-pushed the fix_6910 branch from 05ee2ea to 94e9d9f Compare June 16, 2021 22:02

hashhar reviewed Jun 17, 2021

View reviewed changes

findepi reviewed Jun 17, 2021

View reviewed changes

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java Show resolved Hide resolved

findepi reviewed Jun 17, 2021

View reviewed changes

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java Outdated Show resolved Hide resolved

plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java Show resolved Hide resolved

guyco33 force-pushed the fix_6910 branch from 94e9d9f to 9feb1ff Compare June 17, 2021 08:29

findepi mentioned this pull request Jun 17, 2021

Bump mysql-connector-java to 8.0.22 #8300

Closed

guyco33 force-pushed the fix_6910 branch 3 times, most recently from b926b45 to 1c58ab8 Compare June 21, 2021 13:02

findepi reviewed Jun 22, 2021

View reviewed changes

guyco33 force-pushed the fix_6910 branch from 1c58ab8 to 2136dc8 Compare June 22, 2021 15:11

hashhar approved these changes Jun 22, 2021

View reviewed changes

Support TIMESTAMPS with precisions higher than 3 in MySQL

2b9f4fd

guyco33 force-pushed the fix_6910 branch from 2136dc8 to 2b9f4fd Compare June 23, 2021 12:53

findepi assigned hashhar Jun 24, 2021

hashhar approved these changes Jun 24, 2021

View reviewed changes

hashhar merged commit 48519ff into trinodb:master Jun 28, 2021

hashhar added this to the 359 milestone Jun 28, 2021

This was referenced Jun 28, 2021

Support TIMESTAMPS with precisions higher than 3 in MySQL #6910

Closed

Add support for MySQL datetime data type #5449

Closed

Release notes for 359 #8170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TIMESTAMPS with precisions higher than 3 in MySQL #8060

Support TIMESTAMPS with precisions higher than 3 in MySQL #8060

guyco33 commented May 24, 2021 •

edited

Loading

guyco33 commented May 26, 2021

findepi commented May 31, 2021

hashhar left a comment •

edited

Loading

hashhar left a comment

findepi left a comment

guyco33 Jun 16, 2021

hashhar left a comment

guyco33 commented Jun 17, 2021

findepi Jun 22, 2021

guyco33 Jun 22, 2021

findepi Jun 23, 2021

guyco33 Jun 23, 2021 •

edited

Loading

findepi Jun 22, 2021

guyco33 Jun 22, 2021 •

edited

Loading

findepi Jun 22, 2021

guyco33 Jun 22, 2021

hashhar left a comment

hashhar left a comment

guyco33 commented Jun 27, 2021

hashhar commented Jun 28, 2021

		@@ -605,88 +594,75 @@ public void testTimestampFromMySql(ZoneId sessionZone)
		// before epoch (MySQL's timestamp type doesn't support values <= epoch)
		//.addRoundTrip("timestamp(3)", "TIMESTAMP '1958-01-01 13:18:03.123'", createTimestampType(3), "TIMESTAMP '1958-01-01 13:18:03.000'")

		// epoch (MySQL's timestamp type doesn't support values <= epoch)
		//.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:00:00.000'", createTimestampType(3), "TIMESTAMP '1970-01-01 00:00:00.000'")

Support TIMESTAMPS with precisions higher than 3 in MySQL #8060

Support TIMESTAMPS with precisions higher than 3 in MySQL #8060

Conversation

guyco33 commented May 24, 2021 • edited Loading

guyco33 commented May 26, 2021

findepi commented May 31, 2021

hashhar left a comment • edited Loading

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

guyco33 Jun 16, 2021

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

guyco33 commented Jun 17, 2021

findepi Jun 22, 2021

Choose a reason for hiding this comment

guyco33 Jun 22, 2021

Choose a reason for hiding this comment

findepi Jun 23, 2021

Choose a reason for hiding this comment

guyco33 Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

findepi Jun 22, 2021

Choose a reason for hiding this comment

guyco33 Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

findepi Jun 22, 2021

Choose a reason for hiding this comment

guyco33 Jun 22, 2021

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

hashhar left a comment

Choose a reason for hiding this comment

guyco33 commented Jun 27, 2021

hashhar commented Jun 28, 2021

guyco33 commented May 24, 2021 •

edited

Loading

hashhar left a comment •

edited

Loading

guyco33 Jun 23, 2021 •

edited

Loading

guyco33 Jun 22, 2021 •

edited

Loading