search

trinodb / trino

Official repository of Trino, the distributed SQL query engine for big data, formerly known as PrestoSQL (https://trino.io)
https://trino.io
Apache License 2.0
10.55k stars 3.03k forks source link

More predicate pushdown in ClickHouse connector #7100

Open findepi opened 3 years ago

findepi commented 3 years ago

See TODOs in the code referring to this issue

These include (not an exhaustive list):

JamesRTaylor commented 2 years ago

@findepi - would it be possible to elaborate on what "more pushdown" means? It's not clear which TODOs you mean. Ideally, if we could break this down into specific pushdown improvements that would correspond to individual PRs.

hashhar commented 2 years ago

@JamesRTaylor most of it is regarding untested code (pushdown for text types) and decimal predicates

A quick grep gives me:

$ grep -R -n -E 'TODO.*7100' plugin/trino-clickhouse
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java:409:    // TODO: Remove override once decimal predicate pushdown is implemented (https://github.com/trinodb/trino/issues/7100)
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java:176:                        .add(new ImplementMinMax(false)) // TODO: Revisit once https://github.com/trinodb/trino/issues/7100 is resolved
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java:193:        // TODO: Remove override once https://github.com/trinodb/trino/issues/7100 is resolved. Currently pushdown for textual types is not tested and may lead to incorrect results.
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java:419:                        // TODO (https://github.com/trinodb/trino/issues/7100) Currently pushdown would not work and may require a custom bind expression
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java:431:                // TODO (https://github.com/trinodb/trino/issues/7100) test & enable predicate pushdown
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java:479:                        // TODO (https://github.com/trinodb/trino/issues/7100) fix, enable and test decimal pushdown

I've edited the issue description accordingly to mention some of these.

JamesRTaylor commented 2 years ago

Is pushdown for TIMESTAMP types complete? See here for more context.

ebyhr commented 2 years ago

@JamesRTaylor Predicate pushdown happens on DateTime type, but it doesn't happen on other types (e.g. Datetime64) at this time.

mathsteam commented 2 years ago

Hi, any news for predicate pushdown for textual types? What could be the effects of making textual types as FULL_PUSHDOWN?

hashhar commented 2 years ago

@mathsteam Tests will need to be added to verify the effect and fix any failures accordingly.

See TestPostgreSqlTypeMapping and PostgreSqlConnectorTest#testPredicatePushdown as some examples of what to add to ClickHouse.

tangjiangling commented 2 years ago

Let me do this.

chenjian2664 commented 1 year ago

@tangjiangling Are you still working on textual types pushdown on ClickHouse? If not, I would like to take this

tangjiangling commented 1 year ago

Not at the moment, you can take over.

klimmilk commented 1 year ago

@tangjiangling Are you still working on textual types pushdown on ClickHouse? If not, I would like to take this

Any progress on textual types pushdown? This feature can be also replaced with raw query support, like here https://trino.io/docs/current/connector/postgresql.html#query-varchar-table

style1002 commented 1 year ago

Add ClickHouse automatic cost based JOIN pushdown support #18380 Have any suggestions for #18380

sylph-eu commented 7 months ago

@ebyhr, could you point me what needs to be checked to guarantee correctness of enabling predicate pushdown for textual types (when ClickHouse String is mapped to VARCHAR)?

It's clear how to check that expressions were actually pushed-down (for this I can refer to PostgreSqlConnectorTest#testPredicatePushdown), plus I see examples from TestPostgreSqlTypeMapping to make sure that types are correctly mapped. Given that ClickHouse doesn't support collation, anything else needs to be checked?