Remove fall back to legacy type mapping in Redshift #18209
Conversation
We should not fall back to legacy behavior, the mappings should be explicit (the legacyToWriteMapping is just a copy of some generic default mappings that used to exist)
| @@ -598,8 +598,10 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect | |||
| longTimestampWithTimeZoneWriteFunction())); | |||
| } | |||
|
|
|||
| // Fall back to default behavior | |||
| return legacyToColumnMapping(session, type); | |||
There was a problem hiding this comment.
Can we also remove redshift.use-legacy-type-mapping?
There was a problem hiding this comment.
I'm not sure. I don't know why it was introduced and I don't know what deprecation process does trino have.
There was a problem hiding this comment.
same argument could be given for removal of this line.
There was a problem hiding this comment.
Not exactly. Right now if someone would like to use legacy, they can switch redshift.use-legacy-type-mapping and get it back. This fallback was enhancing proper type mapping with some legacy wrong behaviour.
There was a problem hiding this comment.
Agreed, this is not exactly same.
When do we plan to remove redshift.use-legacy-type-mapping?
There was a problem hiding this comment.
@findepi I'd be glad to remove that code and to answer your questions, but unfortunately I don't know.
I see that this property was added in the commit
Add config property to disable new Redshift type mappings
6fc0b247 Dain Sundstrom <dain@iq80.com> on 23/12/2022 at 00:57
According to https://trino.io/development/process.html#contribution-process
unless the change is trivial, for example a spelling fix in the documentation
it worth to
Start a discussion by creating a GitHub issue, or asking on Slack
Also I'd prefer to merge this PR as soon as possible.
plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftConnectorTest.java
Show resolved
Hide resolved
|
LGTM. please make sure the tests pass before merging |
ssheikin
force-pushed
the
ssheikin/34/trino/redshift_super
branch
from
0b29b1b to
fade80a
Compare
| @@ -101,6 +103,40 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior) | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testSuperColumnType() | |||
There was a problem hiding this comment.
reminder that CI doesn't run this test class at all (not even with /test-with-secrets).
We should not fall back to legacy behavior,
the mappings should be explicit (the legacyToWriteMapping is just a
copy of some generic default mappings that used to exist)
Description
Additional context and related issues
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:
Remove fall back to legacy type mapping in Redshift.