Introduce TrinoToClickHouseWriteChecker in ClickHouse by tangjiangling · Pull Request #11148 · trinodb/trino

tangjiangling · 2022-02-22T19:47:39Z

Description

Different versions of ClickHouse may support different min/max values
for the same data type, you can refer to the table below:

version	column type	min value	max value
any	UInt8	0	255
any	UInt16	0	65535
any	UInt32	0	4294967295
any	UInt64	0	18446744073709551615
< 21.4	Date	1970-01-01	2106-02-07
< 21.4	DateTime	1970-01-01 00:00:00	2106-02-06 06:28:15
>= 21.4	Date	1970-01-01	2149-06-06
>= 21.4	DateTime	1970-01-01 00:00:00	2106-02-07 06:28:15

And when the value written to ClickHouse is out of range, ClickHouse
will store the incorrect result, so we introduced
TrinoToClickHouseWriteChecker to check the range of the written value
to prevent ClickHouse from storing the incorrect value.

Introducing TrinoToClickHouseWriteChecker is also a preparation for
supporting DateTime[(timezone)] and DateTime64(precision, [timezone]).

Fixes part of #11116
Related #10537

Release notes

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

# ClickHouse
* Extend the range of ClickHouse `Date` and `DateTime` values. ({issue}`11116`)

ebyhr

Thanks. Left some initial comments.

docs/src/main/sphinx/connector/clickhouse.rst

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java

.../test/java/io/trino/plugin/clickhouse/TestClickHouseLatestConnectorDateTimesTypeMapping.java

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java

tangjiangling · 2022-02-25T07:19:42Z

Updated, ping @ebyhr

ebyhr

Please rebase on upstream to resolve conflicts.

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseTypeMapping.java

...ino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestLatestClickHouseTypeMapping.java

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java

tangjiangling · 2022-02-26T01:50:43Z

Updated.

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java

tangjiangling · 2022-02-26T20:07:14Z

Minor fixs, PTAL @ebyhr

tangjiangling · 2022-09-06T08:32:13Z

How did we initially discover the issue that writes are loosing values in ClickHouse?

AFAIK it was @ebyhr who discovered this behavior in the first place (see #10055 #11490).

Then I found out that different versions of ClickHouse support different min-max values for date types (inspired by #10055, see #11116).
(DateTime* also has the same issue)

Can you extract last 2 commits into separate PR?

Sure, will do.

tangjiangling · 2022-09-08T16:48:50Z

@hashhar PTAL updated PR(title, description, RN, addressed comments).

Can you extract last 2 commits into separate PR?

Once this PR is merged, I'll file follow-up PRs.

hashhar · 2022-09-08T18:53:58Z

Thanks, I'll take another look tomorrow.

hashhar

LGTM. Please squash the fixup commits.

Different versions of ClickHouse may support different min/max values for the same data type, you can refer to the table below: | version | column type | min value | max value | |---------|-------------|---------------------|----------------------| | any | UInt8 | 0 | 255 | | any | UInt16 | 0 | 65535 | | any | UInt32 | 0 | 4294967295 | | any | UInt64 | 0 | 18446744073709551615 | | < 21.4 | Date | 1970-01-01 | 2106-02-07 | | < 21.4 | DateTime | 1970-01-01 00:00:00 | 2106-02-06 06:28:15 | | >= 21.4 | Date | 1970-01-01 | 2149-06-06 | | >= 21.4 | DateTime | 1970-01-01 00:00:00 | 2106-02-07 06:28:15 | And when the value written to ClickHouse is out of range, ClickHouse will store the incorrect result, so we introduced `TrinoToClickHouseWriteChecker` to check the range of the written value to prevent ClickHouse from storing the incorrect value. Introducing `TrinoToClickHouseWriteChecker` is also a preparation for supporting `DateTime[timezone]` and `DateTime64(precision, [timezone])`. The next several commits will use `TrinoToClickHouseWriteChecker` to verify the values written to ClickHouse.

This is a preparatory commit to use `TrinoToClickHouseWriteChecker` to validate Date.

This is a preparatory commit to use `TrinoToClickHouseWriteChecker` to validate DateTime.

tangjiangling · 2022-09-10T11:28:04Z

Please squash the fixup commits.

done

colebow · 2022-09-13T18:34:18Z

@tangjiangling Do we want a release note for this change? And is there a plan/need to write docs related to this as a follow-up?

cc @hashhar

hashhar · 2022-09-13T18:53:38Z

RN entry is suggested in the PR description. It might be useful to document the supported range of values but it's not urgent since the values will vary and change according to ClickHouse version being used and a useful error message with relevant information is shown in those cases.

colebow · 2022-09-13T19:41:33Z

Oops, somehow missed the release note in the PR description. And that sounds good to me.

zhicwu · 2022-10-18T08:54:51Z

FYI, the date range was changed again in new version - see ClickHouse/clickhouse-java#1103.

tangjiangling · 2022-10-18T09:01:35Z

Thanks for the reminder, we already knew that (see #11148 (comment)) and I will continue this work.

cla-bot bot added the cla-signed label Feb 22, 2022

tangjiangling added the WIP label Feb 22, 2022

tangjiangling force-pushed the add-support-for-datetime64-in-clickhouse branch 3 times, most recently from a0c5748 to 44ed9f4 Compare February 23, 2022 20:21

tangjiangling removed the WIP label Feb 23, 2022

tangjiangling changed the title ~~[WIP] Add support for DateTime64 in ClickHouse~~ Add support for DateTime64 in ClickHouse Feb 23, 2022

tangjiangling requested review from ebyhr and hashhar February 23, 2022 20:23

tangjiangling marked this pull request as ready for review February 23, 2022 20:23

github-actions bot added the docs label Feb 23, 2022

ebyhr reviewed Feb 24, 2022

View reviewed changes

tangjiangling mentioned this pull request Feb 24, 2022

Separate testBasicTypes into different test methods for different column types #11181

Closed

6 tasks

tangjiangling force-pushed the add-support-for-datetime64-in-clickhouse branch from 44ed9f4 to 32f4f1c Compare February 24, 2022 22:55

tangjiangling commented Feb 24, 2022

View reviewed changes

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java Outdated Show resolved Hide resolved

tangjiangling commented Feb 24, 2022

View reviewed changes

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java Outdated Show resolved Hide resolved

tangjiangling force-pushed the add-support-for-datetime64-in-clickhouse branch from 32f4f1c to 8c2f64b Compare February 25, 2022 07:17

ebyhr reviewed Feb 25, 2022

View reviewed changes

tangjiangling force-pushed the add-support-for-datetime64-in-clickhouse branch 2 times, most recently from 51316c0 to f774c1a Compare February 26, 2022 01:48

This was referenced Feb 26, 2022

Minor refactors to SqlServer type mapping tests tangjiangling/trino#1

Closed

Minor refactors to SqlServer type mapping tests #11207

Merged

tangjiangling commented Feb 26, 2022

View reviewed changes

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java Outdated Show resolved Hide resolved

tangjiangling commented Feb 26, 2022

View reviewed changes

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java Outdated Show resolved Hide resolved

tangjiangling commented Feb 26, 2022

View reviewed changes

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java Outdated Show resolved Hide resolved

tangjiangling force-pushed the add-support-for-datetime64-in-clickhouse branch from f774c1a to 885f3ec Compare February 26, 2022 20:05

tangjiangling mentioned this pull request Mar 21, 2022

Use session data provider in Oracle date mapping test #11586

Merged

tangjiangling force-pushed the add-support-for-datetime64-in-clickhouse branch from 4cb5f17 to e55d54e Compare September 8, 2022 16:23

tangjiangling changed the title ~~Add support for DateTime64 in ClickHouse~~ Introduce TrinoToClickHouseWriteChecker in ClickHouse Sep 8, 2022

hashhar approved these changes Sep 10, 2022

View reviewed changes

tangjiangling added 10 commits September 10, 2022 19:22

Use TrinoToClickHouseWriteChecker to validate UInt8

8f0ff68

Use TrinoToClickHouseWriteChecker to validate UInt16

60d0f6b

Use TrinoToClickHouseWriteChecker to validate UInt32

50418c2

Use TrinoToClickHouseWriteChecker to validate UInt64

2670572

Make ClickHouse Date min/max/unsupportedDate tests customizable

dbae902

This is a preparatory commit to use `TrinoToClickHouseWriteChecker` to validate Date.

Extract errorMessageForDateYearOfEraPredicate method

d643abf

Use TrinoToClickHouseWriteChecker to validate Date

bc7c3a3

Make ClickHouse DateTime min/max/unsupportedDateTime tests customizable

d47d34d

This is a preparatory commit to use `TrinoToClickHouseWriteChecker` to validate DateTime.

Use TrinoToClickHouseWriteChecker to validate DateTime

16968ba

tangjiangling force-pushed the add-support-for-datetime64-in-clickhouse branch from 5403be1 to 16968ba Compare September 10, 2022 11:23

hashhar approved these changes Sep 10, 2022

View reviewed changes

hashhar merged commit a02bb52 into trinodb:master Sep 10, 2022

github-actions bot added this to the 396 milestone Sep 10, 2022

tangjiangling deleted the add-support-for-datetime64-in-clickhouse branch September 10, 2022 13:58

This was referenced Sep 11, 2022

Extended ClickHouse Date range #11116

Open

Add support for reading DateTime(timezone) from ClickHouse #14110

Merged

colebow mentioned this pull request Sep 13, 2022

Add Trino 396 release notes #14109

Merged

Conversation

tangjiangling commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

Uh oh!

ebyhr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tangjiangling commented Feb 25, 2022

Uh oh!

ebyhr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tangjiangling commented Feb 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tangjiangling commented Feb 26, 2022

Uh oh!

tangjiangling commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tangjiangling commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashhar commented Sep 8, 2022

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

tangjiangling commented Sep 10, 2022

Uh oh!

colebow commented Sep 13, 2022

Uh oh!

hashhar commented Sep 13, 2022

Uh oh!

colebow commented Sep 13, 2022

Uh oh!

zhicwu commented Oct 18, 2022

Uh oh!

tangjiangling commented Oct 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

tangjiangling commented Feb 22, 2022 •

edited

Loading

tangjiangling commented Sep 6, 2022 •

edited

Loading

tangjiangling commented Sep 8, 2022 •

edited

Loading