Fix serialization for timestamps with precision higher than 3 for MySQL by hashhar · Pull Request #6893 · trinodb/trino

hashhar · 2021-02-15T09:07:29Z

pom.xml

hashhar · 2021-02-16T08:11:03Z

I'll need to disable some test-cases because we currently use SqlTimestamp while writing which means that timestamps lying within a gap do not round-trip.

findepi · 2021-02-16T08:43:41Z

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

The default read function is not correct -- it should either round OR fail, but should not return incorrect data silently as it does today.

I don't think it returns incorrect data - a VerifyException is thrown if the value and the precision don't match.

The issue here was that the value returned from MySQL has precision != 3 and hence the verify fails. It's our fault as callers of the code to not provide the correct TimestampType (we pass TIMESTAMP_MILLIS always).

Should be addressed as part of #6910.

I don't think it returns incorrect data - a VerifyException is thrown if the value and the precision don't match.

Right. That's a new addition (#6654), and so #6852 reported a different stacktrace than one we would get today.

Still, we could add an opt-in mode where the value is rounded (can be eg boolean round)
This should be used by all connectors which use TIMESTAMP_MILLIS (not only MySQL, but also the legacy default mappings (#6607) and maybe some others.

Note that we need to propagate the boolean through legacyToPrestoType -> timestampColumnMapping -> toPrestoTimestamp.

I think it's more worthwhile to add the boolean (or a method with more meaningful name) to toPrestoTimestamp itself and add explicit column mapping for timestamps to connectors which are using legacyToPrestoType.

I would like to do this in a separate PR though.

Note that we need to propagate the boolean through legacyToPrestoType -> timestampColumnMapping -> toPrestoTimestamp.

legacyToPrestoType should not take this boolean. This is the place where we make decision to map to TS_MILLIS.
But yes, you'd need to pass a boolean to timestampColumnMappingUsingSqlTimestamp and on.

findepi · 2021-02-16T08:44:47Z

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

Add atest when data is written with MySQL and read with Trino -- the existing tests should not exercise the read path's rounding, as the data should be stored pre-rounded on the Trino side.

findepi · 2021-02-16T09:03:13Z

I'll need to disable some test-cases because we currently use SqlTimestamp while writing which means that timestamps lying within a gap do not round-trip.

Please do not disable them completely. just use a different input and expected output literals, with a clear TODO comment.
This will ensure we update the test when we fix underlying problem, while the comment ensures no-one will get confused about that.

cc @nineinchnick

hashhar · 2021-02-16T09:51:37Z

I'll need to disable some test-cases because we currently use SqlTimestamp while writing which means that timestamps lying within a gap do not round-trip.

Please do not disable them completely. just use a different input and expected output literals, with a clear TODO comment.

Sorry for the misleading comment, there are no tests today - I meant disabling the ones I was trying to add. Instead I'll add the full set of tests (and fixes) as part of #6910.

findepi · 2021-02-16T10:17:40Z

Sorry for the misleading comment, there are no tests today - I meant disabling the ones I was trying to add.

That's exactly what i understood.

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

jirassimok

Looks mostly good. My most important comments are the last two.

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

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

jirassimok · 2021-02-16T16:23:57Z

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

For tests with rounding, I think it's better to put the actual and expected values on their own lines, so it's easier to see what is expected to change.

.addRoundTrip( "TIMESTAMP '1970-01-01 00:00:00.12345671'", "TIMESTAMP '1970-01-01 01:00:00.123'")

This also works for addRoundTrip(String, String, Type, String), even if the type makes it look a little less nice.

.addRoundTrip( "datetime(6)", "TIMESTAMP '1970-01-01 00:00:01.1234569'", TIMESTAMP_MILLIS, "TIMESTAMP '1970-01-01 01:00:01.123'")

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

hashhar · 2021-02-17T07:50:05Z

AC @findepi @jirassimok

Thanks @jirassimok for catching the rounding/overflow bug.

hashhar · 2021-02-17T08:51:15Z

CI hit #5758.

findepi · 2021-02-17T09:41:38Z

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

The need for rounding applies to MemSQL, Druid, Redshift connectors as well -- i.e. caller of

trino/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

Line 634 in 922bfa1

return Optional.of(timestampColumnMappingUsingSqlTimestamp(TIMESTAMP_MILLIS));

who do not handle timestamps on their own.

That flow used to round until be2b97a, but it went unnoticed until we added more verifications in other places.

MySQL is not a special case here, and we have other connectors suffering -- those who're not smart enough to handle their mapping by themsleves. We should fix the mapping returned from io.trino.plugin.jdbc.StandardColumnMappings#jdbcTypeToPrestoType by rounding to millis. This should be an opt-in, ie it must not be applied to timestampColumnMappingUsingSqlTimestamp or timestampColumnMapping when someone calls them directly.

continued @ #6893 (comment)

Makes sense. I'll change the mapping returned from legacyToPrestoType while leaving the type-level mappings (e.g. timestampColumnMapping as it is.

With rounding enabled all verifyPredicate calls fail for values where we round during reads - most likely because to predicate generated in verifyPredicate is being pushed down.

I'll need to disable pushdown for all connectors using the legacyToPrestoType @findepi.

hashhar · 2021-02-17T12:05:00Z

@findepi AC but notice that pushdown is now disabled for anyone using the rounding timestamp mapping (because rounding happens on our end and hence any pushed-down operations will use wrong value).

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

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

jirassimok · 2021-02-17T20:33:51Z

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

You can make this return ZoneId[] to make the method body look a little nicer.

One of IntelliJ's TestNG inspections ("invalid data provider return type") doesn't like it very much, but IntelliJ is wrong here.

I agree IntelliJ is wrong and its inspection needs an update, but i don't want to have to eye-ball & ignore warnings manually.
So unless we decide the inspection is not helpful and we recommend disabing it for everyone (see DEVELOPING.md), we should avoid triggering warnings.

jirassimok · 2021-02-17T20:42:40Z

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

This also works for addRoundTrip(String, String, Type, String), even if the type makes it look a little less nice.

.addRoundTrip( "datetime(6)", "TIMESTAMP '1970-01-01 00:00:01.1234569'", TIMESTAMP_MILLIS, "TIMESTAMP '1970-01-01 01:00:01.123'")

hashhar · 2021-02-18T10:36:11Z

AC as fixup.

Note that I changed the existing method timestampColumnMappingSqlTimestamp to perform rounding instead of adding a boolean argument/override because I couldn't find any code which directly called this mapping. i.e. all users of this method are the connectors which haven't implemented their own type mappings.

findepi · 2021-02-18T10:30:57Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

The default TIME mapping also misses rounding. Let's solve this separately though.
Maybe revert this for now, as unrelated.

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

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

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

findepi · 2021-02-18T11:22:58Z

As a followup we should add same tests for MemSQL and Druid.

hashhar · 2021-02-18T11:30:44Z

AC as fixups again. Will squash the fixups after approval.

PTAL again @findepi @jirassimok

hashhar · 2021-02-18T11:38:03Z

Squashed the fixups. Should be good to merge on a green build.

findepi · 2021-02-18T12:30:51Z

Please rebase to force CI run

findepi · 2021-02-18T16:25:16Z

@hashhar see CI

Older version of trino-base-jdbc dependency gives serialization error for timestamp column. Issue: trinodb/trino#6852 This bug was fixed by trinodb/trino#6893 Hence updating version was necessary to work with timestamp with precision of more than 3 digits for NetezzaClient as well. Readme is also updated for reference

cla-bot bot added the cla-signed label Feb 15, 2021

hashhar force-pushed the hashhar/mysql-timestamp branch from 1371a8c to ebce4c3 Compare February 15, 2021 09:11

findepi reviewed Feb 15, 2021

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

hashhar force-pushed the hashhar/mysql-timestamp branch from ebce4c3 to 3cb91be Compare February 16, 2021 07:47

findepi reviewed Feb 16, 2021

View reviewed changes

hashhar force-pushed the hashhar/mysql-timestamp branch from 3cb91be to aa8b651 Compare February 16, 2021 14:01

hashhar marked this pull request as ready for review February 16, 2021 14:07

hashhar force-pushed the hashhar/mysql-timestamp branch from aa8b651 to 575186f Compare February 16, 2021 14:08

hashhar requested review from alexjo2144 and jirassimok February 16, 2021 14:11

hashhar force-pushed the hashhar/mysql-timestamp branch 2 times, most recently from c99c8ff to 6937ac4 Compare February 16, 2021 16:01

dain reviewed Feb 16, 2021

View reviewed changes

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

jirassimok reviewed Feb 16, 2021

View reviewed changes

hashhar force-pushed the hashhar/mysql-timestamp branch 2 times, most recently from 94df59b to 94a2100 Compare February 17, 2021 07:13

hashhar requested review from findepi and jirassimok February 17, 2021 07:49

findepi requested changes Feb 17, 2021

View reviewed changes

findepi mentioned this pull request Feb 17, 2021

Remove deprecated unused timestampColumnMapping #6936

Closed

hashhar requested a review from findepi February 17, 2021 12:04

findepi reviewed Feb 17, 2021

View reviewed changes

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

hashhar force-pushed the hashhar/mysql-timestamp branch from d90cf88 to bd322d1 Compare February 17, 2021 13:45

jirassimok reviewed Feb 17, 2021

View reviewed changes

findepi approved these changes Feb 18, 2021

View reviewed changes

hashhar force-pushed the hashhar/mysql-timestamp branch from 35e7288 to e1889ea Compare February 18, 2021 11:37

hashhar force-pushed the hashhar/mysql-timestamp branch from e1889ea to ff10d4d Compare February 18, 2021 12:00

hashhar force-pushed the hashhar/mysql-timestamp branch from ff10d4d to 3cffe1b Compare February 18, 2021 13:23

hashhar added 2 commits February 18, 2021 22:03

Change legacy timestamp mapping to round values and disable pushdown

325fd35

Remove deprecated method

7cfd002

hashhar force-pushed the hashhar/mysql-timestamp branch from 3cffe1b to 7cfd002 Compare February 18, 2021 16:34

findepi merged commit dc1fb53 into trinodb:master Feb 18, 2021

findepi added this to the 353 milestone Feb 18, 2021

findepi mentioned this pull request Feb 18, 2021

Release notes for 353 #6858

Closed

10 tasks

hashhar deleted the hashhar/mysql-timestamp branch February 19, 2021 04:55

jerryleooo mentioned this pull request Jul 7, 2021

Implement timestamp predicate pushdown in Druid connector #8474

Closed

Conversation

hashhar commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hashhar commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Feb 16, 2021

Uh oh!

hashhar commented Feb 16, 2021

Uh oh!

findepi commented Feb 16, 2021

Uh oh!

Uh oh!

jirassimok left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hashhar commented Feb 17, 2021

Uh oh!

hashhar commented Feb 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi commented Feb 18, 2021

Uh oh!

hashhar commented Feb 18, 2021

Uh oh!

hashhar commented Feb 18, 2021

hashhar commented Feb 15, 2021 •

edited

Loading

jirassimok left a comment •

edited

Loading

hashhar Feb 17, 2021 •

edited

Loading

hashhar commented Feb 17, 2021 •

edited

Loading

hashhar commented Feb 18, 2021 •

edited

Loading