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

Jump to bottom

Add varchar/char cast pushdown support in Redshift #23490

Closed
wants to merge 0 commits into from
Closed

Add varchar/char cast pushdown support in Redshift #23490

wants to merge 0 commits into from
+0 −0

Conversation

mayankvadariya
Copy link
Contributor

@mayankvadariya mayankvadariya commented Sep 19, 2024 •
edited
Loading

Description

This PR adds support for cast projection pushdown for the Redshift connector for char/varchar types.

  • char -> char
  • varchar -> char, varchar

Additionally, enforces case-sensitive comparison by using Redshift COLLATE function https://docs.aws.amazon.com/redshift/latest/dg/r_COLLATE.html

This PR depends on #22951

Additional context and related issues

Release notes

( ) This is not user-visible or is 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:

# Section
* Add varchar/char cast pushdown support in Redshift. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Sep 19, 2024
SemionPar
SemionPar reviewed Sep 19, 2024
View reviewed changes
Copy link
Contributor

@SemionPar SemionPar left a comment

Choose a reason for hiding this comment

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

Skimmed

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/BaseRedshiftCastPushdownTest.java Outdated Show resolved Hide resolved
plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/BaseRedshiftCastPushdownTest.java Outdated

new CastTestCase("c_timestamp", "date", Optional.of("c_date")),
new CastTestCase("c_timestamp", "time", Optional.of("c_time")),
new CastTestCase("c_date", "timestamp", Optional.of("c_timestamp")));
Copy link
Contributor

Choose a reason for hiding this comment

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

Some unsupported CASTs to varchar will land here, too, e.g.:
CAST(c_decimal_10_2 AS varchar(50))

@mayankvadariya mayankvadariya force-pushed the mayank/redshift-cast-pushdown branch 3 times, most recently from 3c50921 to 189caa3 Compare September 21, 2024 00:36
@ebyhr
Copy link
Member

ebyhr commented Sep 21, 2024

/test-with-secrets sha=189caa3c735043a9f55c4b81b86ccb0e85217e62

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 21, 2024 •
edited
Loading

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/10968233148

@ebyhr ebyhr mentioned this pull request Sep 23, 2024
Closed
ebyhr
ebyhr reviewed Sep 24, 2024
View reviewed changes
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/ImplementRedshiftLpad.java Outdated
@@ -0,0 +1,97 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain the motivation to support lpad function pusdhown in PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, added.

krvikash
krvikash reviewed Oct 1, 2024
View reviewed changes
Copy link
Contributor

@krvikash krvikash left a comment

Choose a reason for hiding this comment

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

@mayankvadariya can we rebase the PR with master and cherry-pick the latest changes #22951

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/BaseRedshiftCastPushdownTest.java Outdated Show resolved Hide resolved
plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/BaseRedshiftCastPushdownTest.java Outdated Show resolved Hide resolved
plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/BaseRedshiftCastPushdownTest.java Outdated Show resolved Hide resolved
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/ImplementRedshiftLpad.java Outdated Show resolved Hide resolved
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/ImplementRedshiftLpad.java Outdated Show resolved Hide resolved
plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftConnectorTest.java Outdated Show resolved Hide resolved
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/ImplementRedshiftLpad.java Outdated Show resolved Hide resolved
@mayankvadariya mayankvadariya force-pushed the mayank/redshift-cast-pushdown branch 5 times, most recently from 2e202ed to 41ba30e Compare October 8, 2024 19:56
@mayankvadariya mayankvadariya changed the title [WIP] Add varchar/char cast pushdown support in Redshift Add varchar/char cast pushdown support in Redshift Oct 8, 2024
@mayankvadariya mayankvadariya self-assigned this Oct 8, 2024
@mayankvadariya mayankvadariya marked this pull request as ready for review October 8, 2024 20:06
@mayankvadariya mayankvadariya mentioned this pull request Oct 8, 2024
Open
@mayankvadariya
Copy link
Contributor Author

Thanks for the review @krvikash.
Rebased on #22951 and added case-sensitive comparison.

krvikash
krvikash reviewed Oct 9, 2024
View reviewed changes
plugin/trino-redshift/src/main/java/io/trino/plugin/redshift/RewriteCast.java Outdated
Comment on lines 97 to 112
private static boolean supportedSourceTypeToCastToCharType(JdbcTypeHandle sourceType)
{
// varchar -> char is unsupported as varchar supports UNICODE characters but char doesn't.
return switch (sourceType.jdbcType()) {
case CHAR -> true;
default -> false;
};
}

private static boolean supportedSourceTypeToCastToVarcharType(JdbcTypeHandle sourceType)
{
return switch (sourceType.jdbcType()) {
case CHAR, VARCHAR -> true;
default -> false;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we inline it?

plugin/trino-redshift/src/test/java/io/trino/plugin/redshift/TestRedshiftCastPushdown.java Outdated

new CastTestCase("c_varchar_unicode", "varchar(50)", Optional.of("c_varchar_50")),
new CastTestCase("c_nvarchar_unicode", "varchar(50)", Optional.of("c_varchar_50")),

Copy link
Contributor

Choose a reason for hiding this comment

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

can we add test case for cast to varchar (unbounded varchar)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Something like https://github.com/mayankvadariya/trino/blob/41ba30e202dd5ab464d3d6bef7172e518d286db0/plugin/trino-oracle/src/test/java/io/trino/plugin/oracle/TestOracleCastPushdown.java#L411-L445

@ebyhr
Copy link
Member

ebyhr commented Oct 16, 2024

#22951 is merged today. Could you rebase on master to resolve conflicts?

@mayankvadariya
Copy link
Contributor Author

By mistake I closed this PR. Created #23808 as a replacement.

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

@SemionPar SemionPar SemionPar left review comments

@krvikash krvikash krvikash left review comments

@ebyhr ebyhr ebyhr left review comments

@Praveen2112 Praveen2112 Awaiting requested review from Praveen2112

@anusudarsan anusudarsan Awaiting requested review from anusudarsan

Assignees

@mayankvadariya mayankvadariya

Labels
cla-signed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mayankvadariya @ebyhr @krvikash @SemionPar