Improve Clickhouse pushdown. Attempt 1#22921
Improve Clickhouse pushdown. Attempt 1#22921ssheikin wants to merge 5 commits intotrinodb:masterfrom
Conversation
ssheikin
requested review from
Praveen2112,
ebyhr,
hashhar,
sylph-eu and
vlad-lyutenko
ssheikin
changed the title
ssheikin
force-pushed
the
ssheikin/65/trino/clickhouse-minmax
branch
from
194a5df to
233f342
Compare
ssheikin
changed the title
ssheikin
force-pushed
the
ssheikin/65/trino/clickhouse-minmax
branch
5 times, most recently
from
c92db94 to
4592e44
Compare
There was a problem hiding this comment.
Is it something we could restrict the scope inside applyJoin itself ?
There was a problem hiding this comment.
I think it would be a bit tricky -
There was a problem hiding this comment.
I didn't find prettier solution.
There was a problem hiding this comment.
Having different version or ConnectorRewrite for each predicate like filter or join or aggregation might make the framework brittle. Can we implement something like isSupportedJoinCondition for the newer join framework ? Plus doing this change just for one connector i.e Clickhouse doesn't support inequality joins as of now might also be tranisient as Clickhouse might resolve them sometime in the future. This code might loose its importance once it has been implemented.
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
These would be applied for all the data types incl double and real as well - What would be its behaviour wrt to NaN and Infinity ?
There was a problem hiding this comment.
Relates to #23496
pushdowns for real are disabled,
inequalities for double to be tested
There was a problem hiding this comment.
Having different version or ConnectorRewrite for each predicate like filter or join or aggregation might make the framework brittle. Can we implement something like isSupportedJoinCondition for the newer join framework ? Plus doing this change just for one connector i.e Clickhouse doesn't support inequality joins as of now might also be tranisient as Clickhouse might resolve them sometime in the future. This code might loose its importance once it has been implemented.
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
Outdated
Show resolved
Hide resolved
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
I assume you are still running with the @ssheikin and @Praveen2112 .. |
|
Yes |
from convert predicate for filtering, because constructing predicates for join may be more restrictive than for filtering. E.g. in clickhouse https://clickhouse.com/docs/en/sql-reference/statements/select/join#experimental-join-with-inequality-conditions-for-columns-from-different-tables
Assume remote collation is case sensitive Per https://clickhouse.com/docs/en/sql-reference/statements/show#show_columns ClickHouse has no per-column collations Clickhouse is UTF-8 encoded with byte-by-byte comparison. https://clickhouse.com/docs/en/sql-reference/statements/select/order-by#collation-support So exactly as trino. https://github.com/airlift/slice/blob/2.2/src/main/java/io/airlift/slice/Slice.java#L1205 That’s why we can pushdown all operations on varchars. Predicate pushdown - inequality - in - like - not equal (!=, <>) - less than (<) - less than or equal (<=) - greater than (>) - greater than or equal (>=) - between - is null - is not null
Assume remote collation is case sensitive Per https://clickhouse.com/docs/en/sql-reference/statements/show#show_columns ClickHouse has no per-column collations Clickhouse is UTF-8 encoded with byte-by-byte comparison. https://clickhouse.com/docs/en/sql-reference/statements/select/order-by#collation-support So exactly as trino. https://github.com/airlift/slice/blob/2.2/src/main/java/io/airlift/slice/Slice.java#L1205 That’s why we can pushdown all operations on varchars.
ssheikin
force-pushed
the
ssheikin/65/trino/clickhouse-minmax
branch
from
4592e44 to
9e49ef7
Compare
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Show resolved
Hide resolved
ssheikin
changed the title
ssheikin
changed the title
ssheikin
changed the title
5 participants
Description
Per https://clickhouse.com/docs/en/sql-reference/statements/show#show_columns ClickHouse has no per-column collations
Clickhouse is UTF-8 encoded with byte-by-byte comparison. https://clickhouse.com/docs/en/sql-reference/statements/select/order-by#collation-support So exactly as trino.
https://github.com/airlift/slice/blob/2.2/src/main/java/io/airlift/slice/Slice.java#L1205 That’s why we can pushdown all operations on varchars.
Additional context and related issues
Relates to
Superceeded by
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: