Skip to content
/ trino Public
forked from trinodb/trino

Commit

Permalink
Implement remaining TODOs from TIMESTAMP semantic fix
Browse files Browse the repository at this point in the history 
Fixes trinodb#37
  • Loading branch information
@dain
dain committed Feb 6, 2022
1 parent 8b015fd commit 2fa546c
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 21 deletions.
31 changes: 21 additions & 10 deletions client/trino-jdbc/src/test/java/io/trino/jdbc/BaseTestJdbcResultSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,11 +349,17 @@ public void testTime()
.hasMessage("Expected column to be a timestamp type but is time(3)");
});

// TODO https://github.com/trinodb/trino/issues/37
// TODO line 1:8: '00:39:05' is not a valid time literal
// checkRepresentation(statementWrapper.getStatement(), "TIME '00:39:05'", Types.TIME, (rs, column) -> {
// ...
// });
checkRepresentation(connectedStatement.getStatement(), "TIME '00:39:05'", Types.TIME, (rs, column) -> {
assertEquals(rs.getObject(column), toSqlTime(LocalTime.of(0, 39, 5)));
assertEquals(rs.getObject(column, Time.class), toSqlTime(LocalTime.of(0, 39, 5)));
assertThatThrownBy(() -> rs.getDate(column))
.isInstanceOf(SQLException.class)
.hasMessage("Expected value to be a date but is: 00:39:05");
assertEquals(rs.getTime(column), Time.valueOf(LocalTime.of(0, 39, 5)));
assertThatThrownBy(() -> rs.getTimestamp(column))
.isInstanceOf(IllegalArgumentException.class) // TODO (https://github.com/trinodb/trino/issues/5315) SQLException
.hasMessage("Expected column to be a timestamp type but is time(0)");
});

// second fraction could be overflowing to next millisecond
checkRepresentation(connectedStatement.getStatement(), "TIME '10:11:12.1235'", Types.TIME, (rs, column) -> {
Expand Down Expand Up @@ -573,11 +579,16 @@ public void testTimestamp()
assertEquals(rs.getTimestamp(column), Timestamp.valueOf(LocalDateTime.of(1583, 1, 1, 0, 0, 0)));
});

// TODO https://github.com/trinodb/trino/issues/37
// TODO line 1:8: '1970-01-01 00:14:15.123' is not a valid timestamp literal; the expected values will pro
// checkRepresentation(statementWrapper.getStatement(), "TIMESTAMP '1970-01-01 00:14:15.123'", Types.TIMESTAMP, (rs, column) -> {
// ...
// });
checkRepresentation(connectedStatement.getStatement(), "TIMESTAMP '1970-01-01 00:14:15.123'", Types.TIMESTAMP, (rs, column) -> {
assertEquals(rs.getObject(column), Timestamp.valueOf(LocalDateTime.of(1970, 1, 1, 0, 14, 15, 123_000_000)));
assertThatThrownBy(() -> rs.getDate(column))
.isInstanceOf(SQLException.class)
.hasMessage("Expected value to be a date but is: 1970-01-01 00:14:15.123");
assertThatThrownBy(() -> rs.getTime(column))
.isInstanceOf(IllegalArgumentException.class) // TODO (https://github.com/trinodb/trino/issues/5315) SQLException
.hasMessage("Expected column to be a time type but is timestamp(3)");
assertEquals(rs.getTimestamp(column), Timestamp.valueOf(LocalDateTime.of(1970, 1, 1, 0, 14, 15, 123_000_000)));
});

checkRepresentation(connectedStatement.getStatement(), "TIMESTAMP '123456-01-23 01:23:45.123456789'", Types.TIMESTAMP, (rs, column) -> {
assertEquals(rs.getObject(column), Timestamp.valueOf(LocalDateTime.of(123456, 1, 23, 1, 23, 45, 123_456_789)));
Expand Down
7 changes: 3 additions & 4 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/s3select/S3SelectPushdown.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,9 @@ public final class S3SelectPushdown
/*
* Double and Real Types lose precision. Thus, they are not pushed down to S3. Please use Decimal Type if push down is desired.
*
* Pushing down timestamp to s3select is problematic due to following reasons:
* 1) Presto bug: TIMESTAMP behaviour does not match sql standard (https://github.com/trinodb/trino/issues/37)
* 2) Presto uses the timezone from client to convert the timestamp if no timezone is provided, however, s3select is a different service and this could lead to unexpected results.
* 3) ION SQL compare timestamps using precision, timestamps with different precisions are not equal even actually they present the same instant of time. This could lead to unexpected results.
* When S3 select support was added, Trino did not properly implement TIMESTAMP semantic. This was fixed in 2020, and TIMESTAMPS may be supportable now
* (https://github.com/trinodb/trino/issues/10962). Pushing down timestamps to s3select maybe still be problematic due to ION SQL comparing timestamps
* using precision. This means timestamps with different precisions are not equal even actually they present the same instant of time.
*/
private static final Set<String> SUPPORTED_COLUMN_TYPES = ImmutableSet.of(
BOOLEAN_TYPE_NAME,
Expand Down
18 changes: 11 additions & 7 deletions testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,13 +147,16 @@ public void testDateLiterals()
@Test
public void testTimeLiterals()
{
Session chicago = Session.builder(getSession()).setTimeZoneKey(TimeZoneKey.getTimeZoneKey("America/Chicago")).build();
Session kathmandu = Session.builder(getSession()).setTimeZoneKey(TimeZoneKey.getTimeZoneKey("Asia/Kathmandu")).build();

assertEquals(computeScalar("SELECT TIME '3:04:05'"), LocalTime.of(3, 4, 5, 0));
assertEquals(computeScalar("SELECT TIME '3:04:05.123'"), LocalTime.of(3, 4, 5, 123_000_000));
assertQuery("SELECT TIME '3:04:05'");
assertQuery("SELECT TIME '0:04:05'");
// TODO https://github.com/trinodb/trino/issues/37
// TODO assertQuery(chicago, "SELECT TIME '3:04:05'");
// TODO assertQuery(kathmandu, "SELECT TIME '3:04:05'");

assertQuery(chicago, "SELECT TIME '3:04:05'");
assertQuery(kathmandu, "SELECT TIME '3:04:05'");
}

@Test
Expand All @@ -168,13 +171,15 @@ public void testTimeWithTimeZoneLiterals()
@Test
public void testTimestampLiterals()
{
Session chicago = Session.builder(getSession()).setTimeZoneKey(TimeZoneKey.getTimeZoneKey("America/Chicago")).build();
Session kathmandu = Session.builder(getSession()).setTimeZoneKey(TimeZoneKey.getTimeZoneKey("Asia/Kathmandu")).build();

assertEquals(computeScalar("SELECT TIMESTAMP '1960-01-22 3:04:05'"), LocalDateTime.of(1960, 1, 22, 3, 4, 5));
assertEquals(computeScalar("SELECT TIMESTAMP '1960-01-22 3:04:05.123'"), LocalDateTime.of(1960, 1, 22, 3, 4, 5, 123_000_000));
assertQuery("SELECT TIMESTAMP '1960-01-22 3:04:05'");
assertQuery("SELECT TIMESTAMP '1960-01-22 3:04:05.123'");
// TODO https://github.com/trinodb/trino/issues/37
// TODO assertQuery(chicago, "SELECT TIMESTAMP '1960-01-22 3:04:05.123'");
// TODO assertQuery(kathmandu, "SELECT TIMESTAMP '1960-01-22 3:04:05.123'");
assertQuery(chicago, "SELECT TIMESTAMP '1960-01-22 3:04:05.123'");
assertQuery(kathmandu, "SELECT TIMESTAMP '1960-01-22 3:04:05.123'");
}

@Test
Expand Down Expand Up @@ -577,7 +582,6 @@ public void testAssignUniqueId()
@Test
public void testAtTimeZone()
{
// TODO the expected values here are non-sensical due to https://github.com/trinodb/trino/issues/37
assertEquals(computeScalar("SELECT TIMESTAMP '2012-10-31 01:00' AT TIME ZONE INTERVAL '07:09' hour to minute"), zonedDateTime("2012-10-30 18:09:00.000 +07:09"));
assertEquals(computeScalar("SELECT TIMESTAMP '2012-10-31 01:00' AT TIME ZONE 'Asia/Oral'"), zonedDateTime("2012-10-30 16:00:00.000 Asia/Oral"));
assertEquals(computeScalar("SELECT MIN(x) AT TIME ZONE 'America/Chicago' FROM (VALUES TIMESTAMP '1970-01-01 00:01:00+00:00') t(x)"), zonedDateTime("1969-12-31 18:01:00.000 America/Chicago"));
Expand Down

0 comments on commit 2fa546c

Please sign in to comment.