Skip to content
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

Implement remaining TODOs from TIMESTAMP semantic fix #10963

Merged
merged 1 commit into from
Feb 7, 2022
Merged

Implement remaining TODOs from TIMESTAMP semantic fix #10963

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
31 changes: 21 additions & 10 deletions client/trino-jdbc/src/test/java/io/trino/jdbc/BaseTestJdbcResultSet.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Member Author

Choose a reason for hiding this comment

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

The semantics of these values are covered extensively by https://github.com/trinodb/trino/blob/master/core/trino-main/src/test/java/io/trino/operator/scalar/timestamp/TestTimestamp.java#L2639

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