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

Remove support for legacy driver and upgrade to 0.4.1 in ClickHouse connector #16193

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Feb 20, 2023

Description

  • Remove support for legacy driver in ClickHouse since it took more than 1 year after becoming the deprecation
  • Upgrade ClickHouse JDBC driver to 0.4.1. Even the latest driver doesn't provide ResultSetMetaData for query pass-through table function

Fixes #16188

Release notes

(x) Release notes are required, with the following suggested text:

# ClickHouse
* Remove support for legacy JDBC driver. ({issue}`16193`)

@cla-bot cla-bot bot added the cla-signed label Feb 20, 2023
@zhicwu
Copy link
Contributor

zhicwu commented Feb 20, 2023

Hi @ebyhr, can we bump to 0.4.1? Although it's just out for 2 days, but it fixed a few critical bugs found in 0.4.0 like 1) incorrect nested array value; 2) broken serde for Nested data type which may lead to failures when deserializing Nested and JSON values; and 3) bitmap64 support.

Apart from that, I think there's one breaking change worthy of mention here. Starting from 0.4.0, to support binary string(mapping String and FixedString to byte array), you have to explicitly set use_binary_string to true. This is because 1) ~10% performance improvement for deserializing each String column and less memory footprint when use_binary_string=false, which is default; and 2) binary data can be also represented as Array(*Int8) in ClickHouse.

@ebyhr ebyhr force-pushed the ebi/clickhouse-legacy branch from 3d6f0de to 2ca0246 Compare February 20, 2023 23:55
@ebyhr ebyhr changed the title Remove support for legacy driver and upgrade to 0.4.0 in ClickHouse connector Remove support for legacy driver and upgrade to 0.4.1 in ClickHouse connector Feb 20, 2023
@ebyhr
Copy link
Member Author

ebyhr commented Feb 21, 2023

@zhicwu Thanks for sharing the information! Changed to 0.4.1 and enabled use_binary_string property.

@ebyhr
Copy link
Member Author

ebyhr commented Feb 21, 2023

web-ui-checks hit #16179

@ebyhr ebyhr marked this pull request as draft February 21, 2023 03:20
@ebyhr ebyhr force-pushed the ebi/clickhouse-legacy branch from 2ca0246 to 09ef7a2 Compare February 21, 2023 03:37
@zhicwu
Copy link
Contributor

zhicwu commented Feb 21, 2023

@zhicwu Thanks for sharing the information! Changed to 0.4.1 and enabled use_binary_string property.

Thank you and apologize for additional changes at your end. Does it make sense to you to map VARBINARY to Array(UInt8) in ClickHouse?

@ebyhr
Copy link
Member Author

ebyhr commented Feb 21, 2023

@zhicwu What about read logic? Reading Array(UInt8) as VARBINARY looks weird to me.

@zhicwu
Copy link
Contributor

zhicwu commented Feb 21, 2023

@zhicwu What about read logic? Reading Array(UInt8) as VARBINARY looks weird to me.

I'm not joking, but is Array(BYTE) better? BYTE is an alias of Int8.

Looks like the mapped type in ClickHouse doesn't matter when reading/writing varbinary in trino, according to code below:

public static SliceReadFunction varbinaryReadFunction()
{
return (resultSet, columnIndex) -> wrappedBuffer(resultSet.getBytes(columnIndex));
}
public static SliceWriteFunction varbinaryWriteFunction()
{
return SliceWriteFunction.of(Types.VARBINARY, (statement, index, value) -> statement.setBytes(index, value.getBytes()));
}

@ebyhr
Copy link
Member Author

ebyhr commented Feb 22, 2023

@zhicwu Mapping to Array(UInt8) or Array(BYTE) changes the behavior of NULL handling as far as I confirmed. I will keep as-is in this PR. Please feel free to send a new one after this PR.

@ebyhr ebyhr marked this pull request as ready for review February 22, 2023 01:30
@ebyhr ebyhr requested review from hashhar and Praveen2112 February 22, 2023 01:30
@ebyhr ebyhr force-pushed the ebi/clickhouse-legacy branch from 09ef7a2 to 1cbd72c Compare February 24, 2023 22:51
@ebyhr
Copy link
Member Author

ebyhr commented Feb 25, 2023

Addressed comments.

@ebyhr ebyhr force-pushed the ebi/clickhouse-legacy branch from 1cbd72c to 73c63b3 Compare February 26, 2023 23:07
@ebyhr ebyhr requested a review from Praveen2112 February 27, 2023 03:08
@@ -95,6 +95,7 @@
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.BiFunction;

import static com.clickhouse.client.ClickHouseValues.convertToQuotedString;
Copy link
Member

Choose a reason for hiding this comment

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

Do we have this API in the legacy version ?

Copy link
Member

Choose a reason for hiding this comment

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

In this case can we squash with the next commit - I'm happy with both the cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

It existed, but the package was changed in the new version.

@ebyhr ebyhr merged commit 3dd4777 into trinodb:master Feb 27, 2023
@ebyhr ebyhr deleted the ebi/clickhouse-legacy branch February 27, 2023 07:00
@ebyhr ebyhr mentioned this pull request Feb 27, 2023
@github-actions github-actions bot added this to the 409 milestone Feb 27, 2023
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.

upgrade clickhouse jdbc driver to the latest version
3 participants