trino
trino copied to clipboard
Add support for DateTime64 in ClickHouse
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`)
Updated, ping @ebyhr
Updated.
Minor fixs, PTAL @ebyhr
@tangjiangling Sorry for letting this go unattended for so long. Can you please rebase to resolve conflicts and I'll take a look.
@hashhar Updated, and thank you for picking it up and reviewing it.
CI failed due to build docs, I'll take a look.
(Cherry-pick to fix code-style)
Continue this work, it is currently WIP, let me mark it as draft.
@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.
Docs, Date32
and DateTime32
will be supported in the follow-up plan.
https://github.com/ClickHouse/ClickHouse/pull/39425
FYI ClickHouse Date32
& DateTime64
has been expanded again.
CI red, I'll take a look.
(Fixed CI and typo in one of the commit bodies)
(Resolving conflicts and extracting partial commits into #13856)
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.
@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.
Thanks, I'll take another look tomorrow.
Please squash the fixup commits.
done
@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
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.
Oops, somehow missed the release note in the PR description. And that sounds good to me.
FYI, the date range was changed again in new version - see ClickHouse/clickhouse-jdbc#1103.
Thanks for the reminder, we already knew that (see https://github.com/trinodb/trino/pull/11148#issuecomment-1216558987) and I will continue this work.