Use timestamp precision 3 in CTAS in MySQL by jirassimok · Pull Request #7166 · trinodb/trino

jirassimok · 2021-03-05T01:00:17Z

This addresses #6909.

The first two commits are minor refactors to the MySql timestamp tests so they are organized in a way that separates CTAS failures from other failures.

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

hashhar

Thansk for the test method renames. It's much clearer now.

Some changes requested.

hashhar · 2021-03-05T05:41:28Z

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

Move this comment into the javadoc now. And change into the above to testTimestampFromTrino.

Based on this comment, I've dropped this comment entirely. We don't need it to remind us that we'll need more tests when we support more precisions.

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

hashhar · 2021-03-05T05:52:46Z

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

If I am reading correctly, this would only write TIMESTAMP(3) as datetime(3) and fallback to legacy write mapping for other types (which doesn't have a mapping for TIMESTAMP type at all so throwing an "Unsupported column type" error).

Let's change this branch to run for anything that is a TimestampType and try to see if we can write as timestamp(p) where p is the precision obtained from the type. See SqlServerClient for reference.

That's a relatively straightforward change. I was comparing to create table instead of insert, but that way makes more sense.

On second thought, I think this way might be better, especially if we fix #6910 any time soon.

It doesn't really make sense for CTAS to allow more precisions than INSERT, so I think we should always write as DATETIME(3) until #6910.

That leaves us with the option to automatically convert all TIMESTAMPs to precision 3 before inserting on CTAS, but I think that rejecting them is a less surprising behavior, especially if we're soon going to change it to allow other precisions soon.

This is definitely correct change. @hashhar note that other timesamps with other precision are simply not supported here and cannot be created at all

Thanks for catching this @jirassimok and @findepi . I missed that CTAS flow differs from INSERT.
I'll fix it in #6910 .

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

findepi · 2021-03-05T21:53:46Z

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

I don't think it will be reusable, unless MySQL supports picosecond precision

"Reuse" is the wrong word here.

Since we separated these tests into their own method, I think this comment is probably unnecessary. We'll need to add more tests when we support more precisions, which is all that this comment is saying now.

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

findepi · 2021-03-05T22:02:50Z

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

This is definitely correct change. @hashhar note that other timesamps with other precision are simply not supported here and cannot be created at all

- Separate tests that insert timestamps with Trino from tests that insert from MySQL - Move the Trino-insert tests after the MySQL-insert tests - Rename the timestamp test methods to state how they insert data

hashhar

LGTM % leftover TODO comment.

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

hashhar

LGTM.

findepi · 2021-03-08T15:54:32Z

Do we have the same problem for MemSQL?

jirassimok · 2021-03-08T22:58:44Z

Do we have the same problem for MemSQL?

Yes; I'm looking into it.

jirassimok · 2021-03-09T00:06:50Z

MemSQL only supports datetime/timestamp precisions 0 and 6, so this is either the same issue as #5450, or very closely related to it (in the same way #6910 relates to #5449).

cla-bot bot added the cla-signed label Mar 5, 2021

jirassimok commented Mar 5, 2021

View reviewed changes

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

hashhar requested changes Mar 5, 2021

View reviewed changes

findepi changed the title ~~Use timestamp precision 3 in CTAS in MySql~~ Use timestamp precision 3 in CTAS in MySQL Mar 5, 2021

findepi reviewed Mar 5, 2021

View reviewed changes

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

jirassimok force-pushed the mysql-ctas-timestamp branch from 0c69010 to 8f58db6 Compare March 5, 2021 15:33

findepi reviewed Mar 5, 2021

View reviewed changes

Reorganize MySql timestamp type mapping tests

ca804a1

- Separate tests that insert timestamps with Trino from tests that insert from MySQL - Move the Trino-insert tests after the MySQL-insert tests - Rename the timestamp test methods to state how they insert data

jirassimok force-pushed the mysql-ctas-timestamp branch from 8f58db6 to 9a18806 Compare March 5, 2021 22:23

hashhar approved these changes Mar 6, 2021

View reviewed changes

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

hashhar mentioned this pull request Mar 8, 2021

Upgrade MySQL driver to 8.x #7201

Closed

Make CTAS write timestamps to MySql with millisecond accuracy

2f2c19e

jirassimok force-pushed the mysql-ctas-timestamp branch from 9a18806 to 2f2c19e Compare March 8, 2021 14:57

hashhar approved these changes Mar 8, 2021

View reviewed changes

findepi approved these changes Mar 8, 2021

View reviewed changes

findepi merged commit 62c100c into trinodb:master Mar 8, 2021

findepi added this to the 354 milestone Mar 8, 2021

findepi mentioned this pull request Mar 8, 2021

Release notes for 354 #7186

Closed

10 tasks

findepi mentioned this pull request Mar 10, 2021

Disallow use of implicit types in remote databases in type tests #7245

Merged

Conversation

jirassimok commented Mar 5, 2021

Uh oh!

Uh oh!

hashhar left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jirassimok Mar 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

findepi commented Mar 8, 2021

Uh oh!

jirassimok commented Mar 8, 2021

Uh oh!

jirassimok commented Mar 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

jirassimok Mar 5, 2021 •

edited

Loading