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

Enable enum equality predicate pushdown for ClickHouse connector #21585

Closed
wants to merge 1 commit into from

Conversation

sylph-eu
Copy link
Contributor

@sylph-eu sylph-eu commented Apr 17, 2024

Description

Partially addresses #7100 .

Enables equality predicate pushdown for enum types. It's similar to VARCHAR (from Trino's perspective), but is not that "general-purpose". Until ClickHouse fully supports collation (atm it's only implemented for ORDER BY clause) range predicates are not pushed down.

Additional context and related issues

Submitted ClickHouse/ClickHouse#62701 for unexpected behaviour.

Next I'll be looking at Decimals, time types and aggregate functions.

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
* Enable enum equality predicate pushdown. ({issue}`7100`)

@cla-bot cla-bot bot added the cla-signed label Apr 17, 2024
@github-actions github-actions bot added the docs label Apr 17, 2024
@sylph-eu sylph-eu requested review from ebyhr, martint and mosabua April 22, 2024 12:49
@sylph-eu sylph-eu force-pushed the clickhouse_connector_3 branch from 7f3b9d7 to 88618fe Compare April 22, 2024 13:39
@sylph-eu
Copy link
Contributor Author

Rebased.

assertQuerySucceeds(format("SELECT value FROM %s WHERE value = 'hello'", table.getName()));

// ClickHouse shall fail when enum string is not exact
assertQueryFails(format("SELECT value FROM %s WHERE value = 'Hello'", table.getName()), ".*Unknown\\s+element.*\\n*.*");
Copy link
Member

Choose a reason for hiding this comment

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

This query succeeded without this PR's change. I don't think users want to face the query failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with equality pushdown the behavior changes.

I'm thinking of the following workaround to preserve backward compatibility: SELECT value FROM %s WHERE toString(value) = 'Hello', note toString(value). Right now I'm looking at how I can rewrite expression to convert enum columns to string when it's necessary.

Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label May 14, 2024
@sylph-eu sylph-eu force-pushed the clickhouse_connector_3 branch from 88618fe to 2592308 Compare May 21, 2024 11:15
@sylph-eu sylph-eu force-pushed the clickhouse_connector_3 branch from 2592308 to 44e0fcb Compare May 21, 2024 12:26
@github-actions github-actions bot removed the stale label May 21, 2024
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jun 12, 2024
Copy link

github-actions bot commented Jul 4, 2024

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions bot closed this Jul 4, 2024
@mosabua
Copy link
Member

mosabua commented Jul 4, 2024

Feel free to reopen and rebase or start a new PR if you want to continue working on this PR @sylph-eu

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