Add support for reading DateTime(timezone) from ClickHouse#14110
Add support for reading DateTime(timezone) from ClickHouse#14110hashhar merged 2 commits intotrinodb:masterfrom
DateTime(timezone) from ClickHouse#14110Conversation
Reading DateTime from ClickHouse should be mapped to Trino TIMESTAMP(0).
|
(Change 2nd commit title) |
tangjiangling
force-pushed
the
add-support-for-datetime-in-clickhouse
branch
from
e35e101 to
fed6722
Compare
|
(squashed commit) |
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
I'll take one more look at tests.
Some comments meanwhile.
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Outdated
Show resolved
Hide resolved
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
some comments about tests
It'd be useful to take inspiration from probably PostgreSQL type mapping tests for some useful values to test.
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
why only clickhouseCreateAndInsert? trinoCreateAsSelect is also useful since CTAS and INSERT follow different code paths.
There was a problem hiding this comment.
Since Trino's TIMESTAMP(p) WITH TIME ZONE does not know the timezone, we cannot create ClickHouse's DateTime(timezone) through Trino.
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review, I'll address these comments this weekend. |
I'll take a look and make them both as consistent as possible. |
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
LGTM % using literal strings in test
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Outdated
Show resolved
Hide resolved
Previously Trino only supported reading `DateTime` from ClickHouse (no timezone specified) and for `DateTime(timezone)` users had to configure the parameter `unsupported-type-handling` or `jdbc-types-mapped-to-varchar` to support reading of this type. This commit supports reading `DateTime(timezone)` from ClickHouse and maps it to Trino's `TIMESTAMP(0) WITH TIME ZONE`. NOTE: writing data from Trino to ClickHouse `DateTime(timezone)` is not supported.
tangjiangling
force-pushed
the
add-support-for-datetime-in-clickhouse
branch
from
494dfd6 to
9a8cd80
Compare
3 participants
Description
Previously Trino only supported reading
DateTimefrom ClickHouse (notimezone specified) and for
DateTime(timezone)users had to configurethe parameter
unsupported-type-handlingorjdbc-types-mapped-to-varcharto support reading of this type.This commit supports reading
DateTime(timezone)from ClickHouse andmaps it to Trino's
TIMESTAMP(0) WITH TIME ZONE.NOTE: writing data from Trino to ClickHouse
DateTime(timezone)is notsupported.
Fixes #13541
Related #11148 (review)
Release notes
(x) Release notes are required, with the following suggested text: