Redshift connector cannot read timestamp columns in >v341

[grantatspothero]

Querying columns of type timestamp fails in trino versions >v345.

--created is a timestamp column in redshift
select created 
from redshift.schema.table

with the stacktrace:

io.prestosql.spi.PrestoException: Could not serialize column 'created' of type 'timestamp(3)' at position 1:1
	...
Caused by: java.lang.IllegalArgumentException: Expected 0s for digits beyond precision 3: epochMicros = 1545228016441221
	at io.prestosql.spi.type.SqlTimestamp.newInstance(SqlTimestamp.java:51)
	at io.prestosql.spi.type.ShortTimestampType.getObjectValue(ShortTimestampType.java:125)
	at io.prestosql.server.protocol.QueryResultRows.getRowValues(QueryResultRows.java:176)
	... 44 more

I think this is because the redshift connector uses the basejdbc connector, which maps the timestamp redshift type to the TIMESTAMP_MILLIS trino timestamp type. However, the redshift timestamp type has 6 digits of precision (not the 3 trino is expecting).

trino/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

Lines 575 to 577 in 2bcf5d9

	case Types.TIMESTAMP:
	// TODO default to `timestampColumnMapping`
	return Optional.of(timestampColumnMappingUsingSqlTimestamp(TIMESTAMP_MILLIS));

https://docs.aws.amazon.com/redshift/latest/dg/r_Datetime_types.html#r_Datetime_types-timestamp

This started causing queries to fail in v346 when this PR was introduced: #5742 Previously SqlTimestamp. newInstance would round the timestamp when the precision did not match the expected precision, now it explicitly fails.

I think the fix should just be overriding toColumnMapping in the redshift client to handle timestamps with 6 digits of precision.

See this thread for context:
https://trinodb.slack.com/archives/CGB0QHWSW/p1610645172011400

[findepi]

There is lack of rounding in the default column mapping being returned here

trino/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

Line 577 in 03d8e5a

return Optional.of(timestampColumnMappingUsingSqlTimestamp(TIMESTAMP_MILLIS));

which is

trino/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java

Lines 492 to 498 in 03d8e5a

	public static long toPrestoTimestamp(TimestampType timestampType, LocalDateTime localDateTime)
	{
	long precision = timestampType.getPrecision();
	checkArgument(precision <= MAX_SHORT_PRECISION, "Precision is out of range: %s", precision);
	Instant instant = localDateTime.atZone(UTC).toInstant();
	return instant.getEpochSecond() * MICROSECONDS_PER_SECOND + roundDiv(instant.getNano(), NANOSECONDS_PER_MICROSECOND);
	}

The code went in 342.
In 346 we probably added validation (didn't check exact), but the behavior in 342-345 was silently incorrect leading to incorrect query results.

[grantatspothero]

@findepi when you say "silently leading to incorrect query results" do you mean "silently rounding the precision of the timestamp" or do you mean "completely wrong timestamp values". Just trying to grasp the severity of the bug.

[findepi]

@grantatspothero for just SELECT queries it was returning correct results
but if the value was compared with something, or aggregated, it could return incorrect ones.
Of course, i am not saying this is the common case for timestamps, i am just pointing out #5742 is not to the root cause.

[tooptoop4]

clickhouse and druid also using https://github.com/trinodb/trino/blob/388/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/StandardColumnMappings.java#L535

[ebyhr]

Closing as #15365