ClickHouse / clickhouse-java

ClickHouse Java Clients & JDBC Driver
https://clickhouse.com
Apache License 2.0
1.45k stars 537 forks source link

ClientV2 ignoring schema inference hints and infer integer settings #1884

Open alxhill opened 1 month ago

alxhill commented 1 month ago

Describe your feedback

We run the following query through the clickhouse java client:

CREATE TABLE {tmpTableName:Identifier}  ENGINE=Log() AS SELECT * FROM s3('...file.csv', 'CSVWithNames')

With the following QuerySettings:

date_time_input_format=best_effort,
input_format_try_infer_integers=0,
schema_inference_hints='timestamp Nullable(Float64)'
input_format_try_infer_exponent_floats=1,
precise_float_parsing=1,
input_format_try_infer_dates=1,
input_format_try_infer_datetimes=1,
schema_inference_use_cache_for_s3=0,
schema_inference_make_columns_nullable=1

In ClientV1, the created table would have Nullable(Float64) for the timestamp column. ClientV2 seems to ignore the inference hints (and the "input_format_try_infer_integers=0" setting), as the column is Nullable(Int64) instead:

TableSchema{tableName='tmp_csv_0be155a3_920d_4496_87a7_ee2facee2efc', databaseName='nominal', columns=[timestamp Nullable(Int64), a Nullable(Float64), b Nullable(String)], metadata={a={type=Nullable(Float64)}, b={type=Nullable(String)}, timestamp={type=Nullable(Int64)}}, colIndex={a=1, b=2, timestamp=0}, hasDefaults=false}
chernser commented 1 month ago

@alxhill thank you for reporting! will look into it shortly.

alxhill commented 1 week ago

Okay, looks like the culprit is that ClientV1 appends all QuerySettings to the URI, while ClientV2 only appends server settings to the URI.

ClientV1

https://github.com/ClickHouse/clickhouse-java/blob/d9cbaba2c5d4022974787e0691a455374bb8f17a/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ClickHouseHttpConnection.java#L164-L170

ClientV2

https://github.com/ClickHouse/clickhouse-java/blob/d9cbaba2c5d4022974787e0691a455374bb8f17a/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java#L511-L515

Changing from settings.setOption to settings.serverSetting fixed my tests. Seems like a break worth resolving as they're silently being ignored.

chernser commented 6 days ago

@alxhill sorry - my bad - I would need to document this thing.

Is the issue resolved and only documentation should be changed?

Thanks!

alxhill commented 20 hours ago

We are unblocked, but this does seem like a break in the client & not something I would expect to see upgrading between minor versions

chernser commented 20 hours ago

@alxhill sorry about that - my bad. This is consequences of using two different clients under the hood (old and new) . We always keep in mind that changes may be breaking and so we need to handle them properly - why many fixes are getting feature flags.