trino icon indicating copy to clipboard operation
trino copied to clipboard

Add support for DateTime64 in ClickHouse

Open tangjiangling opened this issue 3 years ago • 7 comments

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

ClickHouse connector.

How would you describe this change to a non-technical end user or system administrator?

Add support for DateTime64/32 in ClickHouse.

Related issues, pull requests, and links

Fixes #10537

Documentation

( ) No documentation is needed. (x) Sufficient documentation is included in this PR. ( ) Documentation PR is available with #prnumber. ( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

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

# ClickHouse
* Add support for `datetime64` type. ({issue}`11148`)

tangjiangling avatar Feb 22 '22 19:02 tangjiangling

Updated, ping @ebyhr

tangjiangling avatar Feb 25 '22 07:02 tangjiangling

Updated.

tangjiangling avatar Feb 26 '22 01:02 tangjiangling

Minor fixs, PTAL @ebyhr

tangjiangling avatar Feb 26 '22 20:02 tangjiangling

@tangjiangling Sorry for letting this go unattended for so long. Can you please rebase to resolve conflicts and I'll take a look.

hashhar avatar Apr 25 '22 08:04 hashhar

@hashhar Updated, and thank you for picking it up and reviewing it.

tangjiangling avatar Apr 25 '22 20:04 tangjiangling

CI failed due to build docs, I'll take a look.

tangjiangling avatar Apr 26 '22 03:04 tangjiangling

(Cherry-pick to fix code-style)

tangjiangling avatar Apr 26 '22 07:04 tangjiangling

Continue this work, it is currently WIP, let me mark it as draft.

tangjiangling avatar Aug 11 '22 11:08 tangjiangling

@ebyhr @hashhar I refactored the check section for writing data to ClickHouse to make it more generic, see Introduce TrinoToClickHouseWriteChecker for details.

You can review this section first if you have time, the current status is still WIP.

Things got complicated and I think I should split it into several separate PRs.

tangjiangling avatar Aug 14 '22 23:08 tangjiangling

Docs, Date32 and DateTime32 will be supported in the follow-up plan.

tangjiangling avatar Aug 16 '22 12:08 tangjiangling

https://github.com/ClickHouse/ClickHouse/pull/39425

FYI ClickHouse Date32 & DateTime64 has been expanded again.

tangjiangling avatar Aug 16 '22 12:08 tangjiangling

CI red, I'll take a look.

tangjiangling avatar Aug 16 '22 12:08 tangjiangling

(Fixed CI and typo in one of the commit bodies)

tangjiangling avatar Aug 16 '22 23:08 tangjiangling

(Resolving conflicts and extracting partial commits into #13856)

tangjiangling avatar Aug 26 '22 05:08 tangjiangling

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 avatar Sep 06 '22 08:09 tangjiangling

@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.

tangjiangling avatar Sep 08 '22 16:09 tangjiangling

Thanks, I'll take another look tomorrow.

hashhar avatar Sep 08 '22 18:09 hashhar

Please squash the fixup commits.

done

tangjiangling avatar Sep 10 '22 11:09 tangjiangling

@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

colebow avatar Sep 13 '22 18:09 colebow

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.

hashhar avatar Sep 13 '22 18:09 hashhar

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

colebow avatar Sep 13 '22 19:09 colebow

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

zhicwu avatar Oct 18 '22 08:10 zhicwu

Thanks for the reminder, we already knew that (see https://github.com/trinodb/trino/pull/11148#issuecomment-1216558987) and I will continue this work.

tangjiangling avatar Oct 18 '22 09:10 tangjiangling