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.

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

Add config property to disable new Redshift type mappings #15513

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

dain
Copy link
Member

@dain dain commented Dec 23, 2022

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:

# Redshift
* Implement full type mapping, which can be disable with the `redshift.use-legacy-type-mapping` configuration property ({issue}`15365`)

@dain dain requested a review from martint December 23, 2022 00:00
@cla-bot cla-bot bot added the cla-signed label Dec 23, 2022
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Could you share the manual test result?

@dain
Copy link
Member Author

dain commented Dec 23, 2022

I ran them and they passed

@dain dain merged commit 6fc0b24 into trinodb:master Dec 23, 2022
@dain dain deleted the redshift-legacy-type-mapping branch December 23, 2022 00:50
@github-actions github-actions bot added this to the 404 milestone Dec 23, 2022
Copy link
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

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

typo in release notes:
"which can be disable" -> "which can be disabled"

@@ -237,9 +238,11 @@ public RedshiftClient(
JdbcStatisticsConfig statisticsConfig,
QueryBuilder queryBuilder,
IdentifierMapping identifierMapping,
RemoteQueryModifier queryModifier)
RemoteQueryModifier queryModifier,
Copy link
Member

Choose a reason for hiding this comment

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

nit: queryModifier is always the last arg in *Client classes

@@ -467,6 +470,11 @@ protected void verifyColumnName(DatabaseMetaData databaseMetadata, String column
@Override
public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connection connection, JdbcTypeHandle type)
{
// todo remove this when legacy type mapping is no longer supported
Copy link
Member

Choose a reason for hiding this comment

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

nit: todo -> TODO

return legacyTypeMapping;
}

@Config("redshift.use-legacy-type-mapping")
Copy link
Contributor

Choose a reason for hiding this comment

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

use -> force
It has been used even before. Even more, it's used in other connectors, but as a fallback. Here it overrides improved mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants