Skip to content

Fix TIME type handling in MemSQL #10332

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
ebyhr merged 1 commit into from
Dec 31, 2021
Merged

Fix TIME type handling in MemSQL #10332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 16 additions & 5 deletions plugin/trino-memsql/src/main/java/io/trino/plugin/memsql/MemSqlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import io.trino.spi.type.DecimalType;
import io.trino.spi.type.Decimals;
import io.trino.spi.type.StandardTypes;
import io.trino.spi.type.TimeType;
import io.trino.spi.type.TimestampType;
import io.trino.spi.type.Type;
import io.trino.spi.type.TypeManager;
Expand Down Expand Up @@ -99,7 +100,8 @@
import static io.trino.plugin.jdbc.StandardColumnMappings.shortDecimalWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.smallintColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.smallintWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.timeColumnMappingUsingSqlTime;
import static io.trino.plugin.jdbc.StandardColumnMappings.timeColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.timeWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.timestampColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.timestampWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.tinyintColumnMapping;
Expand All @@ -119,6 +121,7 @@
import static io.trino.spi.type.IntegerType.INTEGER;
import static io.trino.spi.type.RealType.REAL;
import static io.trino.spi.type.SmallintType.SMALLINT;
import static io.trino.spi.type.TimeType.createTimeType;
import static io.trino.spi.type.TimestampType.TIMESTAMP_MICROS;
import static io.trino.spi.type.TimestampType.createTimestampType;
import static io.trino.spi.type.TinyintType.TINYINT;
Expand Down Expand Up @@ -275,7 +278,7 @@ private static Map<String, Integer> getTimestampPrecisions(Connection connection
"FROM information_schema.columns " +
"WHERE table_schema = ? " +
"AND table_name = ? " +
"AND column_type IN ('datetime', 'datetime(6)', 'timestamp', 'timestamp(6)')";
"AND column_type IN ('datetime', 'datetime(6)', 'time', 'time(6)', 'timestamp', 'timestamp(6)')";
try (PreparedStatement statement = connection.prepareStatement(sql)) {
statement.setString(1, tableHandle.getCatalogName());
statement.setString(2, tableHandle.getTableName());
Expand All @@ -284,7 +287,7 @@ private static Map<String, Integer> getTimestampPrecisions(Connection connection
try (ResultSet resultSet = statement.executeQuery()) {
while (resultSet.next()) {
String columnType = resultSet.getString("column_type");
int size = columnType.equals("datetime") || columnType.equals("timestamp") ? 0 : 6;
int size = columnType.equals("datetime") || columnType.equals("time") || columnType.equals("timestamp") ? 0 : 6;
timestampColumnPrecisions.put(resultSet.getString("column_name"), size);
}
}
Expand Down Expand Up @@ -360,8 +363,8 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect
dateReadFunctionUsingLocalDate(),
dateWriteFunction()));
case Types.TIME:
// TODO (https://github.com/trinodb/trino/issues/5450) Fix TIME type mapping
return Optional.of(timeColumnMappingUsingSqlTime());
TimeType timeType = createTimeType(typeHandle.getRequiredDecimalDigits());
return Optional.of(timeColumnMapping(timeType));
case Types.TIMESTAMP:
// TODO (https://github.com/trinodb/trino/issues/5450) Fix DST handling
TimestampType timestampType = createTimestampType(typeHandle.getRequiredDecimalDigits());
Expand Down Expand Up @@ -488,6 +491,14 @@ else if (varcharType.getBoundedLength() <= MEMSQL_MEDIUMTEXT_MAX_LENGTH) {
if (type == DATE) {
return WriteMapping.longMapping("date", dateWriteFunction());
}
if (type instanceof TimeType) {
TimeType timeType = (TimeType) type;
checkArgument(timeType.getPrecision() <= MEMSQL_DATE_TIME_MAX_PRECISION, "The max time precision in MemSQL is 6");
if (timeType.getPrecision() == 0) {
return WriteMapping.longMapping("time", timeWriteFunction(0));
}
return WriteMapping.longMapping("time(6)", timeWriteFunction(6));
}
// TODO implement TIME type
if (type instanceof TimestampType) {
TimestampType timestampType = (TimestampType) type;
Expand Down
9 changes: 7 additions & 2 deletions plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,13 @@ protected Optional<DataMappingTestSetup> filterDataMappingSmokeTestData(DataMapp
return Optional.empty();
}

if (typeName.equals("time")
|| typeName.equals("timestamp(3) with time zone")) {
if (typeName.equals("time")) {
// MemSQL supports only second precision
// Skip 'time' that is alias of time(3) here and add test cases in TestMemSqlTypeMapping.testTime instead
return Optional.empty();
}

if (typeName.equals("timestamp(3) with time zone")) {
return Optional.of(dataMappingTestSetup.asUnsupported());
}

Expand Down
89 changes: 89 additions & 0 deletions plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlTypeMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@
import static io.trino.spi.type.IntegerType.INTEGER;
import static io.trino.spi.type.RealType.REAL;
import static io.trino.spi.type.SmallintType.SMALLINT;
import static io.trino.spi.type.TimeType.TIME_MICROS;
import static io.trino.spi.type.TimeType.TIME_SECONDS;
import static io.trino.spi.type.TimeZoneKey.UTC_KEY;
import static io.trino.spi.type.TimeZoneKey.getTimeZoneKey;
import static io.trino.spi.type.TimestampType.createTimestampType;
Expand Down Expand Up @@ -582,6 +584,93 @@ private DataTypeTest dateTestCases(DataType<LocalDate> dateDataType, ZoneId jvmZ
.addRoundTrip(dateDataType, dateOfLocalTimeChangeBackwardAtMidnightInSomeZone);
}

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

SqlDataTypeTest.create()
.addRoundTrip("time", "TIME '00:00:00'", TIME_MICROS, "TIME '00:00:00.000000'") // default to micro second (same as timestamp) in Trino
.addRoundTrip("time(0)", "NULL", TIME_SECONDS, "CAST(NULL AS time(0))")
.addRoundTrip("time(0)", "TIME '00:00:00'", TIME_SECONDS, "TIME '00:00:00'")
.addRoundTrip("time(0)", "TIME '01:02:03'", TIME_SECONDS, "TIME '01:02:03'")
.addRoundTrip("time(0)", "TIME '23:59:59'", TIME_SECONDS, "TIME '23:59:59'")
.addRoundTrip("time(0)", "TIME '23:59:59.9'", TIME_SECONDS, "TIME '00:00:00'") // round by engine
.addRoundTrip("time(6)", "NULL", TIME_MICROS, "CAST(NULL AS time(6))")
.addRoundTrip("time(6)", "TIME '00:00:00'", TIME_MICROS, "TIME '00:00:00.000000'")
.addRoundTrip("time(6)", "TIME '01:02:03'", TIME_MICROS, "TIME '01:02:03.000000'")
.addRoundTrip("time(6)", "TIME '23:59:59'", TIME_MICROS, "TIME '23:59:59.000000'")
.addRoundTrip("time(6)", "TIME '23:59:59.9'", TIME_MICROS, "TIME '23:59:59.900000'")
.addRoundTrip("time(6)", "TIME '23:59:59.99'", TIME_MICROS, "TIME '23:59:59.990000'")
.addRoundTrip("time(6)", "TIME '23:59:59.999'", TIME_MICROS, "TIME '23:59:59.999000'")
.addRoundTrip("time(6)", "TIME '23:59:59.9999'", TIME_MICROS, "TIME '23:59:59.999900'")
.addRoundTrip("time(6)", "TIME '23:59:59.99999'", TIME_MICROS, "TIME '23:59:59.999990'")
.addRoundTrip("time(6)", "TIME '00:00:00.000000'", TIME_MICROS, "TIME '00:00:00.000000'")
.addRoundTrip("time(6)", "TIME '01:02:03.123456'", TIME_MICROS, "TIME '01:02:03.123456'")
.addRoundTrip("time(6)", "TIME '23:59:59.999999'", TIME_MICROS, "TIME '23:59:59.999999'")
.addRoundTrip("time(6)", "TIME '00:00:00.000000'", TIME_MICROS, "TIME '00:00:00.000000'") // round by engine
.execute(getQueryRunner(), session, trinoCreateAsSelect("tpch.test_time"))
.execute(getQueryRunner(), session, trinoCreateAndInsert(getSession(), "tpch.test_time"));

SqlDataTypeTest.create()
.addRoundTrip("time", "NULL", TIME_SECONDS, "CAST(NULL AS time(0))") // default to second in MemSQL
.addRoundTrip("time", "'00:00:00'", TIME_SECONDS, "TIME '00:00:00'")
.addRoundTrip("time", "'01:02:03'", TIME_SECONDS, "TIME '01:02:03'")
.addRoundTrip("time", "'23:59:59'", TIME_SECONDS, "TIME '23:59:59'")
.addRoundTrip("time", "'23:59:59.9'", TIME_SECONDS, "TIME '23:59:59'") // MemSQL ignores millis and stores only seconds in 'time' type
.addRoundTrip("time(6)", "NULL", TIME_MICROS, "CAST(NULL AS time(6))")
.addRoundTrip("time(6)", "'00:00:00'", TIME_MICROS, "TIME '00:00:00.000000'")
.addRoundTrip("time(6)", "'01:02:03'", TIME_MICROS, "TIME '01:02:03.000000'")
.addRoundTrip("time(6)", "'23:59:59'", TIME_MICROS, "TIME '23:59:59.000000'")
.addRoundTrip("time(6)", "'23:59:59.9'", TIME_MICROS, "TIME '23:59:59.900000'")
.addRoundTrip("time(6)", "'23:59:59.99'", TIME_MICROS, "TIME '23:59:59.990000'")
.addRoundTrip("time(6)", "'23:59:59.999'", TIME_MICROS, "TIME '23:59:59.999000'")
.addRoundTrip("time(6)", "'23:59:59.9999'", TIME_MICROS, "TIME '23:59:59.999900'")
.addRoundTrip("time(6)", "'23:59:59.99999'", TIME_MICROS, "TIME '23:59:59.999990'")
.addRoundTrip("time(6)", "'00:00:00.000000'", TIME_MICROS, "TIME '00:00:00.000000'")
.addRoundTrip("time(6)", "'01:02:03.123456'", TIME_MICROS, "TIME '01:02:03.123456'")
.addRoundTrip("time(6)", "'23:59:59.999999'", TIME_MICROS, "TIME '23:59:59.999999'")
.addRoundTrip("time(6)", "'23:59:59.9999999'", TIME_MICROS, "TIME '23:59:59.999999'") // MemSQL ignores nanos and stores only micros in 'time(6)' type
.execute(getQueryRunner(), session, memSqlCreateAndInsert("tpch.test_time"));
}

@Test(dataProvider = "unsupportedTimeDataProvider")
public void testUnsupportedTime(String unsupportedTime)
{
try (TestTable table = new TestTable(memSqlServer::execute, "tpch.test_unsupported_time", "(col time)", ImmutableList.of(format("'%s'", unsupportedTime)))) {
assertQueryFails(
"SELECT * FROM " + table.getName(),
format("\\Q%s cannot be parse as LocalTime (format is \"HH:mm:ss[.S]\" for data type \"TIME\")", unsupportedTime));
}

try (TestTable table = new TestTable(memSqlServer::execute, "tpch.test_unsupported_time", "(col time(6))", ImmutableList.of(format("'%s'", unsupportedTime)))) {
assertQueryFails(
"SELECT * FROM " + table.getName(),
format("\\Q%s.000000 cannot be parse as LocalTime (format is \"HH:mm:ss[.S]\" for data type \"TIME\")", unsupportedTime));
}
}

@DataProvider
public Object[][] unsupportedTimeDataProvider()
Comment thread
ebyhr marked this conversation as resolved.
{
return new Object[][] {
{"-838:59:59"}, // min value in MemSQL
{"-00:00:01"},
{"24:00:00"},
{"838:59:59"}, // max value in MemSQL
};
}

@Test(dataProvider = "unsupportedDateTimePrecisions")
public void testUnsupportedTimePrecision(int precision)
{
// This test should be fixed if future MemSQL supports those precisions
assertThatThrownBy(() -> memSqlServer.execute(format("CREATE TABLE test_unsupported_timestamp_precision (col1 TIME(%s))", precision)))
.hasMessageContaining("Feature 'TIME type with precision other than 0 or 6' is not supported by MemSQL.");
}

@Test(dataProvider = "sessionZonesDataProvider")
public void testDatetime(ZoneId sessionZone)
{
Expand Down