Add support for DateTime64(precision[, timezone]) in ClickHouse #26905
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit supports reading `DateTime64(precision, timezone)` from ClickHouse. In addition to this, previously Trino `Timestamp(0)` was mapped to ClickHouse `DateTime`, this commit extends this behavior by mapping Trino `Timestamp(p)` to ClickHouse `DateTime64(p)`. Co-authored-by: Alex Jo <[email protected]>
Reviewer's GuideThis pull request adds full support for ClickHouse DateTime64(precision[, timezone]) by extending Trino’s TimestampType mapping to leverage DateTime64(p), introducing new read/write functions with sub-second precision and validation, refactoring tests to cover all precisions and boundaries, and updating connector documentation accordingly. Entity relationship diagram for updated ClickHouse-Trino type mappingerDiagram
ClickHouse_DateTime64 {
int precision
string timezone
}
Trino_Timestamp {
int precision
}
ClickHouse_DateTime64 ||--|| Trino_Timestamp : maps to
ClickHouse_DateTime {
string timezone
}
Trino_Timestamp_0 {
}
ClickHouse_DateTime ||--|| Trino_Timestamp_0 : maps to
Class diagram for updated timestamp mapping and write functionsclassDiagram
class ClickHouseClient {
+int CLICKHOUSE_MAX_SUPPORTED_DATETIME64_PRECISION
+Optional<ColumnMapping> toColumnMapping(...)
+WriteMapping toWriteMapping(...)
+ColumnMapping timestampColumnMapping(TimestampType, ClickHouseVersion)
+ColumnMapping timestampWithTimeZoneColumnMapping(ClickHouseColumn, int, ClickHouseVersion)
+LongWriteFunction shortTimestampWriteFunction(ClickHouseVersion)
+ObjectWriteFunction longTimestampWriteFunction(ClickHouseVersion, int)
+LongWriteFunction shortTimestampWithTimeZoneWriteFunction(TrinoToClickHouseWriteChecker<LocalDateTime>, ClickHouseVersion, TimeZone)
+ObjectReadFunction longTimestampWithTimeZoneReadFunction()
+ObjectWriteFunction longTimestampWithTimeZoneWriteFunction()
}
class TrinoToClickHouseWriteChecker<T> {
+static DATETIME64: TrinoToClickHouseWriteChecker<LocalDateTime>
}
class TimestampType {
+int getPrecision()
+static int MAX_SHORT_PRECISION
+static int MAX_PRECISION
}
class ColumnMapping {
+static ColumnMapping longMapping(...)
+static ColumnMapping objectMapping(...)
}
class WriteMapping {
+static WriteMapping longMapping(...)
+static WriteMapping objectMapping(...)
}
class LongTimestamp {
+long getEpochMicros()
+long getPicosOfMicro()
}
class LongTimestampWithTimeZone {
+long getEpochMillis()
+long getPicosOfMilli()
+TimeZoneKey getTimeZoneKey()
+static fromEpochSecondsAndFraction(...)
}
ClickHouseClient --> TrinoToClickHouseWriteChecker
ClickHouseClient --> TimestampType
ClickHouseClient --> ColumnMapping
ClickHouseClient --> WriteMapping
ClickHouseClient --> LongTimestamp
ClickHouseClient --> LongTimestampWithTimeZone
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java:757` </location>
<code_context>
- // TODO (https://github.com/trinodb/trino/issues/10537) Add support for Datetime64 type
- return Optional.of(timestampColumnMapping(TIMESTAMP_MILLIS));
+ if (columnDataType == ClickHouseDataType.DateTime64) {
+ return Optional.of(timestampColumnMapping(createTimestampType(typeHandle.requiredDecimalDigits()), getClickHouseServerVersion(session)));
+ }
+ // TODO Add support for Datetime32 type
</code_context>
<issue_to_address>
**issue (bug_risk):** Check for requiredDecimalDigits exceeding supported precision.
Validate requiredDecimalDigits against CLICKHOUSE_MAX_SUPPORTED_DATETIME64_PRECISION before using it to avoid errors or data loss.
</issue_to_address>
### Comment 2
<location> `plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java:826-830` </location>
<code_context>
- if (type == TIMESTAMP_SECONDS) {
- return WriteMapping.longMapping("DateTime", timestampSecondsWriteFunction(getClickHouseServerVersion(session)));
+ if (type instanceof TimestampType timestampType) {
+ int precision = min(timestampType.getPrecision(), CLICKHOUSE_MAX_SUPPORTED_DATETIME64_PRECISION);
+ if (precision <= TimestampType.MAX_SHORT_PRECISION) {
+ return WriteMapping.longMapping(format("DateTime64(%d)", precision), shortTimestampWriteFunction(getClickHouseServerVersion(session)));
+ }
+ return WriteMapping.objectMapping(format("DateTime64(%d)", precision), longTimestampWriteFunction(getClickHouseServerVersion(session), precision));
}
if (type.equals(uuidType)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider handling precision values below 0.
If timestampType.getPrecision() returns a negative value, min will not prevent invalid precision. Please add a check to ensure precision is non-negative before using it.
</issue_to_address>
### Comment 3
<location> `plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/TrinoToClickHouseWriteChecker.java:73-80` </location>
<code_context>
new TimestampWriteValueChecker(
version -> version.isNewerOrEqualTo("21.4"),
new Range<>(LocalDateTime.parse("1970-01-01T00:00:00"), LocalDateTime.parse("2106-02-07T06:28:15")))));
+ public static final TrinoToClickHouseWriteChecker<LocalDateTime> DATETIME64 = new TrinoToClickHouseWriteChecker<>(
+ ImmutableList.of(
+ new TimestampWriteValueChecker(
+ version -> version.isOlderThan("21.4"),
+ new Range<>(LocalDateTime.parse("1900-01-01T00:00:00"), LocalDateTime.parse("2262-04-11T23:47:16"))),
+ new TimestampWriteValueChecker(
+ version -> version.isNewerOrEqualTo("21.4"),
+ new Range<>(LocalDateTime.parse("1900-01-01T00:00:00"), LocalDateTime.parse("2262-04-11T23:47:16")))));
private final List<Checker<T>> checkers;
</code_context>
<issue_to_address>
**issue:** Consider aligning DateTime64 range with documented values.
Please verify that the DateTime64 range matches the documented values for each version, and update the code or comments to ensure consistency.
</issue_to_address>
### Comment 4
<location> `plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java:1053-1056` </location>
<code_context>
+ }
+
@Test
public void testUnsupportedTimestamp()
{
- testUnsupportedTimestamp("1969-12-31 23:59:59"); // MIN_SUPPORTED_DATETIME_VALUE - 1 second
- testUnsupportedTimestamp("2106-02-07 06:28:16"); // MAX_SUPPORTED_DATETIME_VALUE + 1 second
+ testUnsupportedTimestamp("1899-12-31 23:59:59"); // MIN_SUPPORTED_DATETIME_VALUE - 1 second
+ testUnsupportedTimestamp("2262-04-11 23:47:17"); // MAX_SUPPORTED_DATETIME_VALUE + 1 second
}
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertions for error messages when unsupported timestamps are used with DateTime64.
Please add assertions to verify that the error messages returned for unsupported DateTime64 timestamps are both specific and informative.
Suggested implementation:
```java
@Test
public void testUnsupportedTimestamp()
{
assertUnsupportedTimestampError("1899-12-31 23:59:59", "DateTime64 value 1899-12-31 23:59:59 is out of supported range"); // MIN_SUPPORTED_DATETIME_VALUE - 1 second
assertUnsupportedTimestampError("2262-04-11 23:47:17", "DateTime64 value 2262-04-11 23:47:17 is out of supported range"); // MAX_SUPPORTED_DATETIME_VALUE + 1 second
}
```
```java
private void assertUnsupportedTimestampError(String timestamp, String expectedMessage)
{
try {
testUnsupportedTimestamp(timestamp);
org.junit.Assert.fail("Expected exception for unsupported timestamp: " + timestamp);
}
catch (Exception e) {
org.junit.Assert.assertTrue(
"Error message should mention unsupported DateTime64 value",
e.getMessage().contains(expectedMessage)
);
}
}
@Test
```
If `testUnsupportedTimestamp` does not throw an exception with a message, you may need to update its implementation to do so, or to propagate the error message so it can be asserted. Adjust the expected error message string as needed to match the actual error thrown by your codebase.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // TODO (https://github.com/trinodb/trino/issues/10537) Add support for Datetime64 type | ||
| return Optional.of(timestampColumnMapping(TIMESTAMP_MILLIS)); | ||
| if (columnDataType == ClickHouseDataType.DateTime64) { | ||
| return Optional.of(timestampColumnMapping(createTimestampType(typeHandle.requiredDecimalDigits()), getClickHouseServerVersion(session))); |
There was a problem hiding this comment.
issue (bug_risk): Check for requiredDecimalDigits exceeding supported precision.
Validate requiredDecimalDigits against CLICKHOUSE_MAX_SUPPORTED_DATETIME64_PRECISION before using it to avoid errors or data loss.
Comment on lines
+826
to
+830
| int precision = min(timestampType.getPrecision(), CLICKHOUSE_MAX_SUPPORTED_DATETIME64_PRECISION); | ||
| if (precision <= TimestampType.MAX_SHORT_PRECISION) { | ||
| return WriteMapping.longMapping(format("DateTime64(%d)", precision), shortTimestampWriteFunction(getClickHouseServerVersion(session))); | ||
| } | ||
| return WriteMapping.objectMapping(format("DateTime64(%d)", precision), longTimestampWriteFunction(getClickHouseServerVersion(session), precision)); |
There was a problem hiding this comment.
issue (bug_risk): Consider handling precision values below 0.
If timestampType.getPrecision() returns a negative value, min will not prevent invalid precision. Please add a check to ensure precision is non-negative before using it.
Comment on lines
+73
to
+80
| public static final TrinoToClickHouseWriteChecker<LocalDateTime> DATETIME64 = new TrinoToClickHouseWriteChecker<>( | ||
| ImmutableList.of( | ||
| new TimestampWriteValueChecker( | ||
| version -> version.isOlderThan("21.4"), | ||
| new Range<>(LocalDateTime.parse("1900-01-01T00:00:00"), LocalDateTime.parse("2262-04-11T23:47:16"))), | ||
| new TimestampWriteValueChecker( | ||
| version -> version.isNewerOrEqualTo("21.4"), | ||
| new Range<>(LocalDateTime.parse("1900-01-01T00:00:00"), LocalDateTime.parse("2262-04-11T23:47:16"))))); |
There was a problem hiding this comment.
issue: Consider aligning DateTime64 range with documented values.
Please verify that the DateTime64 range matches the documented values for each version, and update the code or comments to ensure consistency.
alexjo2144
commented
Oct 9, 2025
| new TimestampWriteValueChecker( | ||
| version -> version.isNewerOrEqualTo("21.4"), | ||
| new Range<>(LocalDateTime.parse("1970-01-01T00:00:00"), LocalDateTime.parse("2106-02-07T06:28:15"))))); | ||
| public static final TrinoToClickHouseWriteChecker<LocalDateTime> DATETIME64 = new TrinoToClickHouseWriteChecker<>( |
There was a problem hiding this comment.
Could probably dedup this, not sure why it was split into pre/post 21.4. The values are the same.
I did widen these ranges from the original PR based on the documentation I found. https://clickhouse.com/docs/sql-reference/data-types/datetime64
I also went with the lower upper range value. Technically it could be higher when using a precision < 9.
|
Just noticed there was this attempt too #23788 |
|
cc: @krvikash |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This commit supports reading
DateTime64(precision, timezone)from ClickHouse.In addition to this, previously Trino
Timestamp(0)was mapped to ClickHouseDateTime, this commit extends this behavior by mapping TrinoTimestamp(p)to ClickHouseDateTime64(p).Additional context and related issues
Re-opening an old issue and PR @tangjiangling was working on some time ago:
#10537
#15040
Besides resolving some merge conflicts and a few tweaks this is all his work.
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:
Summary by Sourcery
Add full support for ClickHouse DateTime64 types by mapping Trino TIMESTAMP(p) to DateTime64(p), implementing read/write logic with precision handling and range checks, and updating tests and documentation accordingly
New Features:
Enhancements:
Documentation:
Tests: