Replace deprecated driver ru.yandex.clickhouse.ClickHouseDriver#10559
Replace deprecated driver ru.yandex.clickhouse.ClickHouseDriver#10559tangjiangling wants to merge 4 commits intotrinodb:masterfrom tangjiangling:replace-clickhouse-driver
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
|
cc @ebyhr |
|
@ebyhr i have encountered some testing problems, please help ~ |
ebyhr
left a comment
ebyhr
left a comment
There was a problem hiding this comment.
Error: src/main/java/io/trino/plugin/clickhouse/ClickHouseClientModule.java:[29,1] (imports) ImportOrder: Wrong order for 'com.clickhouse.jdbc.ClickHouseDriver' import.
Please fix import order. Style guide exists in https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#code-style
I guess "No suitable ClickHouse client" comes from clickhouse-http-client exclusion in pom.xml.
|
Got it. |
|
@ebyhr When I removed the exclusion Have these test codes been tested? |
|
@tangjiangling The error message mean you need to debug why new tables don't appear after the creation. |
|
Oh i see, i will do that. |
@ebyhr The reason for this error is: After replacing
From the above diagram we can see: the ClickHouse (version 20.8.19.4) used in the test has no Why is there no problem running the test with SQL used in SQL used in Because |
|
Does this mean that the driver isn't backward compatible (and considered a bug in ClickHouse JDBC driver?)? |
Yes, it is a compatibility issue.
Yes, I'm going to send this question back to the clickhouse-jdbc community and see if I can get some feedback. |
|
Sent a PR to clickhouse-jdbc repository ClickHouse/clickhouse-java#804 |
|
@ebyhr @hashhar Another problem: When I upgraded the ClickHouse version to 21.11 (changing
We need to be more careful with tableTypes, for example in the picture above we actually want to get the table |
|
@tangjiangling Could you add smoke tests with |
|
@tangjiangling you can pass either an empty string array or even null as table types instead. As to pom.xml, I think it's better to use shaded jar for least dependency and avoid potential conflict, for example: <dependency>
<groupId>com.clickhouse</groupId>
<artifactId>clickhouse-jdbc</artifactId>
<version>0.3.2-patch1</version>
<!-- chance classifier to 'all' if you need experimental gRPC support -->
<classifier>http</classifier>
<exclusions>
<exclusion>
<groupId>*</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>I'll release |
@ebyhr Sorry for the delay. I noticed that you have added smoke tests(#10600). So next I need to wait for your PR to merge to master and then update the current PR? |
I will update the current PR when this version is released. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
Thanks. Just released |
The driver `ru.yandex.clickhouse.ClickHouseDriver` has been marked as deprecated. Also everything in package `ru.yandex.clickhouse` will be removed starting from 0.4.0.
|
[WARN] This is a draft PR. TODOs:
I will update the current PR again once the two issues above have been fixed. |
When we replace `ru.yandex.clickhouse.ClickHouseDriver` with `com.clickhouse.jdbc.ClickHouseDriver`, the `com.clickhouse.jdbc.internal.ClickHouseConnectionImpl#getCatalog` method already returns `null`, so there's no need to override the `getTables` method.
This fixes: - Implement the `getTableTypes` method in `io.trino.plugin.clickhouse.ClickHouseClient` - Set the correct `SliceReadFunction` for `String` or `FixedString` types when `mapStringAsVarchar` is not set - Expected error messages in class `io.trino.plugin.clickhouse.TestClickHouseConnectorTest`
|
Clickhouse 20.8 is end of life. The minimal supported Clickhouse version is 21.3. |
This is true in vanilla ClickHouse, but EOL of 20.8 in Altinity is 02 December 2022. https://docs.altinity.com/altinitystablebuilds/ edit: I added tests for Altinity. It's ready to upgrade the ClickHouse image version. |
Does it make sense to use private final GenericContainer<?> dockerContainer;
...
dockerContainer = new GenericContainer<>(CLICKHOUSE_IMAGE).withExposedPorts(HTTP_PORT)
.waitingFor(Wait.forHttp("/ping").forPort(HTTP_PORT).forStatusCode(200)
.withStartupTimeout(Duration.of(60, SECONDS)));
As discussed, I created a new PR to fix the issue, see ClickHouse/clickhouse-java#815
I tried to run all the tests using master branch but ended up with below exception. Not sure if I did something wrong. Could you re-run test at your end with above two changes? |
Sounds good. cc @ebyhr |
Okay, I will take a look. |
|
@tangjiangling We would like to verify the connection using at query level before running tests, so |
Yes, I will take your advice and then see if there are any other problems. |
|
Could you also remove |
@zhicwu Sorry for the late reply. The reason for this error I have found before, you can read this. I will test and collect all compatibility issues with the new driver by this Sunday and then put it together and submit it to you. |
|
I think you need to consider some special cases (e.g. : |
@zhicwu After testing, I found the following problems:
I've updated my PR and you can review it for me. |
|
Thanks for the feedback @tangjiangling! Please see my comments below inline.
I guess we should return scale for timestamp(DateTime, Datetime64(scale)) as well. However, it seems trino expects
Good catch! Besides, the assumption of using UTF8 charset will ruin binary data. Below is an attempt to fix that but I'm seeking a compact format like in Trino(
Didn't see that in CI but I'll definitely enhance the test by adding more descriptions. |
I will update this PR soon, and you can test it with the branch corresponding to this PR. |
Yes, this will cause this test to fail. (https://github.com/trinodb/trino/blob/master/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseTypeMapping.java#L386) |
Any concern to make your repo public? Will try it out tonight. |
I mistakenly deleted the branch corresponding to the current PR (both local and remote were overwritten) and now I can't seem to update the current PR.
In order to support binary data, We need to implement |
@ebyhr Is there a remedy for this situation? Or do I have to file a new PR? o(╥﹏╥)o |
You can use this branch to test. |
|
@tangjiangling, I updated ClickHouse/clickhouse-java#815, test against your branch, and it's all green now :) Could you double check at your end and let me know if there's more issue? I'll need to run a few benchmarks tomorrow and I'll probably update the PR accordingly. If no other issue shows up, I'm going to release patch3 by tomorrow. Also I want to take the chance to thank you all for your help and efforts for integrating the new driver. Please feel free to let me know how I can help to expedite. Lastly, below are some items I think you guys may want to consider before closing this PR:
<dependency>
<groupId>com.clickhouse</groupId>
<artifactId>clickhouse-jdbc</artifactId>
<classifier>all</classifier>
<version>0.3.2-patch3</version>
<exclusions>
<exclusion>
<groupId>*</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
|
I'll do that. |
Great, I tested it and all results are also green. Thank you for doing this.
I will update current PR as soon as the new patch3 is released. |
Thanks for confirming. 0.3.2-patch3 is available in maven central now, cheers :) |
5 participants
Fixes: #10541
The driver
ru.yandex.clickhouse.ClickHouseDriverhas been marked as deprecated.Also everything in package
ru.yandex.clickhousewill be removed starting from 0.4.0.Here are some things I've done for review:
com.clickhouse.jdbc.ClickHouseDriverandru.yandex.clickhouse.ClickHouseDriverru.yandex.clickhouse.ClickHouseDriverplugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse