Conversation
There was a problem hiding this comment.
Is this related to adding tests?
There was a problem hiding this comment.
Yes it is needed for the tests to run
There was a problem hiding this comment.
This is not limited to tests - this applies to the connection factory itself and is meant to improve performance.
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftTableStatisticsReader.java
Outdated
Show resolved
Hide resolved
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RedshiftTableStatisticsReader.java
Outdated
Show resolved
Hide resolved
| <excludes> | ||
| <exclude>**/TestRedshiftAutomaticJoinPushdown.java</exclude> | ||
| <exclude>**/TestRedshiftConnectorTest.java</exclude> | ||
| <exclude>**/TestRedshiftTableStatisticsReader.java</exclude> | ||
| <exclude>**/TestRedshiftTypeMapping.java</exclude> | ||
| </excludes> |
There was a problem hiding this comment.
no profile which includes it? How can I run the tests manually (outside of IDE)?
| } | ||
|
|
||
| // Redshift avg function rounds down resulting decimal. | ||
| // To match Presto avg semantics, we extend scale by 1 and round result to target scale. |
| DecimalType type = (DecimalType) columnHandle.getColumnType(); | ||
| verify(aggregateFunction.getOutputType().equals(type)); | ||
|
|
||
| // When decimal type has maximum precision we can get result that is not matching Presto avg semantics. |
There was a problem hiding this comment.
This is not limited to tests - this applies to the connection factory itself and is meant to improve performance.
Add this to each commit's message. |
| @@ -23,12 +23,22 @@ | |||
| <artifactId>trino-base-jdbc</artifactId> | |||
There was a problem hiding this comment.
This change is backward incompatible change. It is changing semantics of Redshift connector. One could say that after this change it is a new different Redshift connector. Most of the existing production usages of Redshift will now require a migration to make Trino upgrade. Such migration is a binary step. Either user is adjusted to use 403 Redshift connector or 404. There is no middle ground, so rolling back such migration is also difficult.
There was a problem hiding this comment.
Let's restore old one as redshift-legacy?
There was a problem hiding this comment.
I am not sure if it is enough. I would prefer to rename the existing one to mark it is deprecated (legacy) and create a new one with unique name (redshift-v2?). Then after some time once legacy is removed I would rename the one to redshift)
Done |
6 participants
Description
This PR contains many improvements for Redshift with a full test suite. Currently the test suite must be run manually.
I have run the test manually and they all pass.
Fixes #14697
This was co-authored by: @alexjo2144 @ebyhr @findepi @findinpath @grantatspothero @hashhar @jirassimok @kokosing @losipiuk @Praveen2112 @raunaqmorarka @skrzypo987 @ssheikin @wendigo
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text: