Skip to content

Use timestamp precision 3 in CTAS in MySQL #7166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
findepi merged 2 commits into from
Mar 8, 2021
Merged

Use timestamp precision 3 in CTAS in MySQL #7166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca804a1
Reorganize MySql timestamp type mapping tests
jirassimok Mar 4, 2021
2f2c19e
Make CTAS write timestamps to MySql with millisecond accuracy
jirassimok Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion plugin/trino-mysql/src/main/java/io/trino/plugin/mysql/MySqlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
}
if (TIMESTAMP_MILLIS.equals(type)) {
// TODO use `timestampWriteFunction` (https://github.com/trinodb/trino/issues/6910)
return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP_MILLIS));
return WriteMapping.longMapping("datetime(3)", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP_MILLIS));

@hashhar hashhar Mar 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jirassimok jirassimok Mar 5, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jirassimok jirassimok Mar 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@findepi findepi Mar 5, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hashhar hashhar Mar 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if (VARBINARY.equals(type)) {
return WriteMapping.sliceMapping("mediumblob", varbinaryWriteFunction());
Expand Down
66 changes: 39 additions & 27 deletions plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,8 +481,11 @@ public void testDate()
}
}

/**
* Read {@code DATATIME}s inserted by MySQL as Trino {@code TIMESTAMP}s
*/
@Test(dataProvider = "sessionZonesDataProvider")
public void testDatetime(ZoneId sessionZone)
Comment thread
jirassimok marked this conversation as resolved.
Outdated
public void testMySqlDatetimeType(ZoneId sessionZone)
{
Session session = Session.builder(getSession())
.setTimeZoneKey(TimeZoneKey.getTimeZoneKey(sessionZone.getId()))
Expand Down Expand Up @@ -589,38 +592,16 @@ public void testDatetime(ZoneId sessionZone)
.execute(getQueryRunner(), session, mysqlCreateAndInsert("tpch.test_timestamp"));
}

/**
* Read {@code TIMESTAMP}s inserted by MySQL as Trino {@code TIMESTAMP}s
*/
@Test(dataProvider = "sessionZonesDataProvider")
public void testTimestamp(ZoneId sessionZone)
public void testTimestampFromMySql(ZoneId sessionZone)
{
Session session = Session.builder(getSession())
.setTimeZoneKey(TimeZoneKey.getTimeZoneKey(sessionZone.getId()))
.build();

// TODO we are dropping precision during writes because everything is mapped to datetime(0) (https://github.com/trinodb/trino/issues/6909)
SqlDataTypeTest.create()
// before epoch
.addRoundTrip("timestamp(3)", "TIMESTAMP '1958-01-01 13:18:03.123'", TIMESTAMP_MILLIS, "TIMESTAMP '1958-01-01 13:18:03.000'")
// after epoch
.addRoundTrip("timestamp(3)", "TIMESTAMP '2019-03-18 10:01:17.987'", TIMESTAMP_MILLIS, "TIMESTAMP '2019-03-18 10:01:18.000'")
// time doubled in JVM zone
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-10-28 01:33:17.456'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-10-28 01:33:17.000'")
// time double in Vilnius
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-10-28 03:33:33.333'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-10-28 03:33:33.000'")
// epoch
.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:00:00.000'", TIMESTAMP_MILLIS, "TIMESTAMP '1970-01-01 01:00:00.000'")
// TODO time gaps do not round-trip (https://github.com/trinodb/trino/issues/6910)
.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:13:42.000'", TIMESTAMP_MILLIS, "TIMESTAMP '1970-01-01 01:13:42.000'")
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-04-01 02:13:55.123'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-04-01 03:13:55.000'")
// time gap in Vilnius
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-03-25 03:17:17.000'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-03-25 03:17:17.000'")
// time gap in Kathmandu
.addRoundTrip("timestamp(3)", "TIMESTAMP '1986-01-01 00:13:07.000'", TIMESTAMP_MILLIS, "TIMESTAMP '1986-01-01 00:13:07.000'")

.execute(getQueryRunner(), session, trinoCreateAsSelect(session, "test_timestamp"))
.execute(getQueryRunner(), session, trinoCreateAsSelect(getSession(), "test_timestamp"))
.execute(getQueryRunner(), session, trinoCreateAndInsert(session, "test_timestamp"));

// TODO merge this into the above SqlDataTypeTest once we support writing and reading timestamps with precisions other than 3 (TODO https://github.com/trinodb/trino/issues/6910)
// Same as above but with inserts from MySQL - i.e. read path
SqlDataTypeTest.create()
// before epoch (MySQL's timestamp type doesn't support values <= epoch)
Expand Down Expand Up @@ -719,6 +700,37 @@ public void testTimestamp(ZoneId sessionZone)
.execute(getQueryRunner(), session, mysqlCreateAndInsert("tpch.test_timestamp"));
}

@Test(dataProvider = "sessionZonesDataProvider")
public void testTimestampFromTrino(ZoneId sessionZone)
{
Session session = Session.builder(getSession())
.setTimeZoneKey(TimeZoneKey.getTimeZoneKey(sessionZone.getId()))
.build();

SqlDataTypeTest.create()
// before epoch
.addRoundTrip("timestamp(3)", "TIMESTAMP '1958-01-01 13:18:03.123'", TIMESTAMP_MILLIS, "TIMESTAMP '1958-01-01 13:18:03.123'")
// after epoch
.addRoundTrip("timestamp(3)", "TIMESTAMP '2019-03-18 10:01:17.987'", TIMESTAMP_MILLIS, "TIMESTAMP '2019-03-18 10:01:17.987'")
// time doubled in JVM zone
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-10-28 01:33:17.456'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-10-28 01:33:17.456'")
// time double in Vilnius
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-10-28 03:33:33.333'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-10-28 03:33:33.333'")
// epoch
.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:00:00.000'", TIMESTAMP_MILLIS, "TIMESTAMP '1970-01-01 01:00:00.000'")
// TODO time gaps do not round-trip (https://github.com/trinodb/trino/issues/6910)
.addRoundTrip("timestamp(3)", "TIMESTAMP '1970-01-01 00:13:42.000'", TIMESTAMP_MILLIS, "TIMESTAMP '1970-01-01 01:13:42.000'")
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-04-01 02:13:55.123'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-04-01 03:13:55.123'")
// time gap in Vilnius
.addRoundTrip("timestamp(3)", "TIMESTAMP '2018-03-25 03:17:17.000'", TIMESTAMP_MILLIS, "TIMESTAMP '2018-03-25 03:17:17.000'")
// time gap in Kathmandu
.addRoundTrip("timestamp(3)", "TIMESTAMP '1986-01-01 00:13:07.000'", TIMESTAMP_MILLIS, "TIMESTAMP '1986-01-01 00:13:07.000'")

.execute(getQueryRunner(), session, trinoCreateAsSelect(session, "test_timestamp"))
.execute(getQueryRunner(), session, trinoCreateAsSelect(getSession(), "test_timestamp"))
.execute(getQueryRunner(), session, trinoCreateAndInsert(session, "test_timestamp"));
}

@DataProvider
public Object[][] sessionZonesDataProvider()
{
Expand Down