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

Fix serialization for timestamps with precision higher than 3 for MySQL #6893

Merged
merged 2 commits into from
Feb 18, 2021
Merged

Fix serialization for timestamps with precision higher than 3 for MySQL #6893

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
325fd35
Change legacy timestamp mapping to round values and disable pushdown
hashhar Feb 16, 2021
7cfd002
Remove deprecated method
hashhar Feb 17, 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
29 changes: 16 additions & 13 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,27 +434,30 @@ public static LongWriteFunction timeWriteFunction(int precision)
/**
* @deprecated This method uses {@link java.sql.Timestamp} and the class cannot represent date-time value when JVM zone had
* forward offset change (a 'gap'). This includes regular DST changes (e.g. Europe/Warsaw) and one-time policy changes
* (Asia/Kathmandu's shift by 15 minutes on January 1, 1986, 00:00:00). If driver only supports {@link LocalDateTime}, use
* {@link #timestampColumnMapping} instead.
* (Asia/Kathmandu's shift by 15 minutes on January 1, 1986, 00:00:00). This mapping also disables pushdown by default
* to ensure correctness because rounding happens within Trino and won't apply to remote system.
* If driver supports {@link LocalDateTime}, use {@link #timestampColumnMapping} instead.
*/
@Deprecated
public static ColumnMapping timestampColumnMappingUsingSqlTimestamp(TimestampType timestampType)
public static ColumnMapping timestampColumnMappingUsingSqlTimestampWithRounding(TimestampType timestampType)
{
// TODO support higher precision
checkArgument(timestampType.getPrecision() <= TimestampType.MAX_SHORT_PRECISION, "Precision is out of range: %s", timestampType.getPrecision());
return ColumnMapping.longMapping(
timestampType,
(resultSet, columnIndex) -> {
Timestamp timestamp = resultSet.getTimestamp(columnIndex);
return toPrestoTimestamp(timestampType, timestamp.toLocalDateTime());
LocalDateTime localDateTime = resultSet.getTimestamp(columnIndex).toLocalDateTime();
int roundedNanos = toIntExact(round(localDateTime.getNano(), 9 - timestampType.getPrecision()));
LocalDateTime rounded = localDateTime
.withNano(0)
.plusNanos(roundedNanos);
return toPrestoTimestamp(timestampType, rounded);
},
timestampWriteFunctionUsingSqlTimestamp(timestampType));
}

@Deprecated
public static ColumnMapping timestampColumnMapping()
{
return timestampColumnMapping(TIMESTAMP_MILLIS);
timestampWriteFunctionUsingSqlTimestamp(timestampType),
// NOTE: pushdown is disabled because the values stored in remote system might not match the values as
// read by Trino due to rounding. This can lead to incorrect results if operations are pushed down to
// the remote system.
DISABLE_PUSHDOWN);
}

public static ColumnMapping timestampColumnMapping(TimestampType timestampType)
Expand Down Expand Up @@ -631,7 +634,7 @@ public static Optional<ColumnMapping> jdbcTypeToPrestoType(JdbcTypeHandle typeHa

case Types.TIMESTAMP:
// TODO default to `timestampColumnMapping`
return Optional.of(timestampColumnMappingUsingSqlTimestamp(TIMESTAMP_MILLIS));
return Optional.of(timestampColumnMappingUsingSqlTimestampWithRounding(TIMESTAMP_MILLIS));
}
return Optional.empty();
}
Expand Down
6 changes: 5 additions & 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 @@ -310,6 +310,10 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect

case Types.DATE:
return Optional.of(dateColumnMapping());

case Types.TIMESTAMP:
// TODO support higher precisions (https://github.com/trinodb/trino/issues/6910)
break; // currently handled by the default mappings
}

// TODO add explicit mappings
Expand Down Expand Up @@ -355,7 +359,7 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
throw new TrinoException(NOT_SUPPORTED, "Unsupported column type: " + type.getDisplayName());
}
if (TIMESTAMP_MILLIS.equals(type)) {
// TODO use `timestampWriteFunction`
// TODO use `timestampWriteFunction` (https://github.com/trinodb/trino/issues/6910)
return WriteMapping.longMapping("datetime", timestampWriteFunctionUsingSqlTimestamp(TIMESTAMP_MILLIS));
}
if (VARBINARY.equals(type)) {
Expand Down
Loading