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

Support Clickhouse varchar comparison pushdown #23558

Conversation

ssheikin
Copy link
Contributor

@ssheikin ssheikin commented Sep 25, 2024

Description

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:

# Clickhouse
* Improve varchar comparison pushdown for complex expression ({issue}`23558`)

@cla-bot cla-bot bot added the cla-signed label Sep 25, 2024
@ssheikin ssheikin requested review from hashhar, sylph-eu and Praveen2112 and removed request for hashhar September 25, 2024 10:53
@ssheikin ssheikin force-pushed the ssheikin/51/trino/clickhouse-varchar-comparison-pushdown branch from 42ab5c4 to 6cdf3fc Compare September 25, 2024 11:14
Comment on lines +949 to +959
a_enum_1 Enum('hello', 'world', 'a', 'b', 'c'),
a_enum_2 Enum('hello', 'world', 'a', 'b', 'c'))
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to a dedicated test for enums ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was initially implemented as a dedicated test, however it is duplicated in this test anyway to support:

            assertThat(query("SELECT some_column FROM " + table.getName() + " WHERE a_string = a_string_alias")).isFullyPushedDown();
            assertThat(query("SELECT some_column FROM " + table.getName() + " WHERE a_string = a_string_alias" + withConnectorExpression)).isFullyPushedDown();
            assertThat(query("SELECT some_column FROM " + table.getName() + " WHERE a_string = a_enum_1")).isNotFullyPushedDown(FilterNode.class);
            assertThat(query("SELECT some_column FROM " + table.getName() + " WHERE a_string = a_enum_1" + withConnectorExpression)).isNotFullyPushedDown(FilterNode.class);

scenarios. So let it be as it is.

@ssheikin ssheikin force-pushed the ssheikin/51/trino/clickhouse-varchar-comparison-pushdown branch from 6cdf3fc to 15585ed Compare September 25, 2024 12:12
@ssheikin
Copy link
Contributor Author

@Praveen2112 please approve

Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

LGTM % changes on method name and additional test coverage for a few data types.

@ssheikin ssheikin force-pushed the ssheikin/51/trino/clickhouse-varchar-comparison-pushdown branch 2 times, most recently from ffd6822 to d774d3a Compare September 25, 2024 15:15
@ssheikin
Copy link
Contributor Author

@Praveen2112 all comments are addressed.

Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

@Praveen2112
Copy link
Member

@hashhar / @ebyhr Do you want to take another round of review for this PR.

@ssheikin ssheikin force-pushed the ssheikin/51/trino/clickhouse-varchar-comparison-pushdown branch from 6498658 to 3d8a123 Compare September 25, 2024 15:59
@ssheikin ssheikin deleted the ssheikin/51/trino/clickhouse-varchar-comparison-pushdown branch September 26, 2024 13:37
@mosabua mosabua mentioned this pull request Sep 27, 2024
1 task
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.

3 participants