Add ClickHouse connector

[wgzhao]

I deleted former PR( #4672) in my stupid operation.

The ClickHouse connector allows querying data stored in
Yandex ClickHouse

The ClickHouse connector is compatible with all ClickHouse versions starting from 20.3.5.21.

to view more details in presto-docs/clichouse.rst

Fixes #4500

[findepi]

@wgzhao could you possibly squash your commits and make sure the build is green?

[Praveen2112]

Can we have this on top of #5012 so that we can push table properties without extending JDBC metadata

[electrum]

@wgzhao It looks like this is close. Can you rebase as @Praveen2112 suggested and squash the commits?

[wgzhao]

@wgzhao It looks like this is close. Can you rebase as @Praveen2112 suggested and squash the commits?

#5012 has not merge, can I use it directly?

[Praveen2112]

@wgzhao #5012 is merged.. So now we could rebase your PR

[findepi]

@wgzhao can you please rebase and squash all commits into one?

[findepi]

can you please rebase and squash all commits into one?

@wgzhao Please see https://docs.github.com/en/github/using-git/about-git-rebase
When you do this, the "Commits" tab of the PR (https://github.com/prestosql/presto/pull/4909/commits) should show just 1 commit.

BTW thanks for making sure the build is green!
@Praveen2112 can you take another look?

[findepi]

@wgzhao note there is a compilation error in ClickHouseQueryBuilder

2020-09-14T10:18:30.9881571Z [ERROR] /home/runner/work/presto/presto/presto-clickhouse/src/main/java/io/prestosql/plugin/clickhouse/ClickHouseQueryBuilder.java:[99,15] incompatible types: java.lang.String cannot be converted to io.prestosql.plugin.jdbc.JdbcClient

[Praveen2112]

@wgzhao I guess there is a small issue in merging. I'm not sure if InMemoryOrcDataSource and other changes are a part of this PR

[wgzhao]

@wgzhao I guess there is a small issue in merging. I'm not sure if InMemoryOrcDataSource and other changes are a part of this PR

Yes, I remove all mistaken merging and re-commit

[wgzhao]

can you please rebase and squash all commits into one?

@wgzhao Please see https://docs.github.com/en/github/using-git/about-git-rebase
When you do this, the "Commits" tab of the PR (https://github.com/prestosql/presto/pull/4909/commits) should show just 1 commit.

BTW thanks for making sure the build is green!
@Praveen2112 can you take another look?

Thank you , I have squash all commits into one commit

[rosewms]

Looks good to me, from a docs perspective.

[findepi]

@wgzhao please let me know when you address the comments. Also just FYI, the build is red currently.

[wgzhao]

@wgzhao please let me know when you address the comments. Also just FYI, the build is red currently.

I try my best to address above comments on next week, thanks

[findepi]

@wgzhao are you working on any updates or is now waiting for my review?

[wgzhao]

@wgzhao are you working on any updates or is now waiting for my review?

If it's convenient for you now, you can review first, we are still in the Chinese New Year holiday

[findepi]

@wgzhao i am taking a closer look at type mappings and was trying to tie up some loose ends here

However i realized i do not fully understand the idea behind String and FixedString in Clickhouse
Can you help me understand this more?

From the documentation that i found for String https://clickhouse.tech/docs/en/sql-reference/data-types/string/

Strings of an arbitrary length. The length is not limited. The value can contain an arbitrary set of bytes, including null bytes.
The String type replaces the types VARCHAR, BLOB, CLOB, and others from other DBMSs.

And from FixedString docs https://clickhouse.tech/docs/en/sql-reference/data-types/fixedstring/ :

A fixed-length string of N bytes (neither characters nor code points).

To me it sounds like we should map the both types to varbinary, and not varchar or char on the Trino side, otherwise some values (those which happen not to be UTF-8 encoded) may not be accessible.
However, I can imagine docs saying one thing, and everyone using these types to store UTF-8 data only. Is it the case?

[wgzhao]

FixedString DOES have a confusing behavior, from the official documentation, the usage scenario is mainly those data stored in fixed length with ASCII , such as IP addresses, hashes for MD5, etc.
The underlying is not stored with UTF8 encoding. But in fact, it can store UTF-8 data, and and be queried, such as the following example:

:) create table t (col FixedString(5)) ENGINE=TinyLog;
:) insert into t values('hell'),('中');

:) select * from t;

SELECT *
FROM t

┌─col──┐
│ hell │
│ 中   │
└──────┘

:) select * from t where col = '中\0\0';

SELECT *
FROM t
WHERE col = '中\0\0'

┌─col─┐
│ 中  │
└─────┘

My suggestion is for FixedString only for one-sided mapping, that is, when querying ClickHouse table, mapping FixedString(N) type to CHAR(N) on Trino,
but when querying, for string whose length is less than N, it needs to be padded with \0 (see the above example), which may confuse user.
When creating the ClickHouse table on the Trino side, map the CHAR type to the String type on clickhouse.

[hashhar]

Some comments.

You also need to add an entry to https://github.com/trinodb/trino/blob/master/core/trino-server/src/main/provisio/presto.xml to make sure the connector gets packaged into the final JAR.

[hashhar]

You can solve this by providing your own implementation of getTableHandle.
The JdbcTableHandle created in getTableHandle takes the catalog name as input.

Take a look at MySqlClient for example.

[hashhar]

This will also allow you to remove most of the overrides here.

[wgzhao]

I tried it, But it brings up more errors

[hashhar]

Override is not needed. If your JdbcTableHandle sets catalogname to empty optional then BaseJdbcClient#renameTable automatically constructs the proper SQL (including quoting).

[wgzhao]

BaseJdbcClient#renameTable use ALTER TABLE RENAME TO statement, But ClickHouse uses a different statment RENAME TABLE TO. So, I had to override it.

[findepi]

@wgzhao I applied most of @hashhar's comments and tweaked the type mapping tests a bit.
I wanted to understand why some of the tests were not using SqlDataTypeTest.

Also, i adapted the connector tests to the new base test class i recently introduced -- i didn't want to add more work on you because of changes done in the meantime.

I also took liberty to swap the String, FixedString mapping into varbinary and added configuration toggle clickhouse.map-string-as-varchar to restore varchar mapping behavior.
This is an interm solution and we should reconsider the mapping, see #7102

I posted #7081 and will merge when green.

As this is the first version of the connector, there naturally is a bunch of things to take care of as follow ups.
I tried to capture them in #7103. If I missed something, let me know.

Thank you for your contribution, you did great job here and I am excited to see ClickHouse connector shipping soon!

[findepi]

Thanks @wgzhao for updates here. I am copying them into #7081 as well.

[findepi]

Merged as 174d429.

@wgzhao many thanks for the contribution and it so cool to see it being released soon!

[wgzhao]

Thanks