Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support DateTime64 in ClickHouse #23788

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Support DateTime64 in ClickHouse #23788

wants to merge 1 commit into from
+151 −44

Conversation

ssheikin
Copy link
Contributor

@ssheikin ssheikin commented Oct 15, 2024 •
edited
Loading

Description

Additional context and related issues

PRs:

ClickHouse docs:

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.
(x) Release notes are required, with the following suggested text:

## Section
* Support DateTime64 in ClickHouse. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Oct 15, 2024
@ssheikin ssheikin requested review from Praveen2112, ebyhr and hashhar and removed request for Praveen2112 and ebyhr October 15, 2024 16:51
This was referenced Oct 15, 2024
Open
Merged
Open
@ssheikin ssheikin requested a review from ebyhr October 15, 2024 17:00
@ssheikin ssheikin force-pushed the ssheikin/43/trino/clickhouse-datetime64 branch from d62c0bd to 4ef6297 Compare October 16, 2024 07:29
@ssheikin ssheikin mentioned this pull request Oct 16, 2024
Open
@ebyhr ebyhr removed their request for review October 24, 2024 06:08
Praveen2112
Praveen2112 reviewed Nov 11, 2024
View reviewed changes
Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check with the test code

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 Show resolved Hide resolved
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java Show resolved Hide resolved
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
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

private static WriteMapping timestampWithTimeZoneWriteMapping(TimestampWithTimeZoneType timestampWithTimeZoneType)
{
verify(timestampWithTimeZoneType.getPrecision() <= CLICKHOUSE_MAX_SUPPORTED_TIMESTAMP_PRECISION);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this check a bit redundant ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be useful. In Trino MAX_PRECISION = 12 for TimestampWithTimeZoneType.

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
plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java Show resolved Hide resolved
@ssheikin ssheikin force-pushed the ssheikin/43/trino/clickhouse-datetime64 branch from 4ef6297 to bce8a64 Compare November 18, 2024 13:05
@github-actions github-actions bot added the docs label Nov 18, 2024
@ssheikin
Copy link
Contributor Author

@Praveen2112 comments are addressed. PTAL.

@ssheikin ssheikin force-pushed the ssheikin/43/trino/clickhouse-datetime64 branch from bce8a64 to bf96aaa Compare November 21, 2024 13:39
@ssheikin
Copy link
Contributor Author

@Praveen2112 comments are addressed, PTAL.

@ssheikin
Support DateTime64 in ClickHouse
20cefa3 
TIMESTAMP(p)
TIMESTAMP(p) WITH TIME ZONE
@ssheikin ssheikin force-pushed the ssheikin/43/trino/clickhouse-datetime64 branch from bf96aaa to 20cefa3 Compare November 21, 2024 14:18
Praveen2112
Praveen2112 reviewed Nov 26, 2024
View reviewed changes
plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
Comment on lines 805 to 807
if (type == TIMESTAMP_SECONDS) {
return WriteMapping.longMapping("DateTime", timestampSecondsWriteFunction(getClickHouseServerVersion(session)));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be merged with TIMESTAMP_SECONDS -

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you are asking about these cases:

        if (type == TIMESTAMP_SECONDS) {
            return WriteMapping.longMapping("DateTime", timestampSecondsWriteFunction(getClickHouseServerVersion(session)));
        }
        if (type instanceof TimestampType timestampType) {
            return timestampWriteMapping(timestampType);
        }

They could be merged, but TimestampType returns native type as DateTime64(%s), whereas TIMESTAMP_SECONDS -> DateTime without precision.
The default processing may be changed later via additional property.
As this change is invasive let's reconsider it in separate PR if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this moment may never come, so will address in this PR.

plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java
return WriteMapping.longMapping(dataType, timestampWriteFunction(createTimestampType(precision)));
}
checkArgument(precision <= CLICKHOUSE_MAX_SUPPORTED_TIMESTAMP_PRECISION, "Precision is out of range: %s", precision);
return WriteMapping.objectMapping(dataType, longTimestampWriteFunction(type, precision));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to handle the min and max value like we handle for seconds right ?

plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/BaseClickHouseTypeMapping.java
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add multiple precision for testUnsupportedTimestamp

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

private SqlDataTypeTest dateTimeWithTimeZoneTest(Function<ZoneId, String> inputTypeFactory)
private void addTestCasesForClickhouseCreateAndTrinoInsert(SqlDataTypeTest tests, Function<String, String> inputTypeFactory, int precision)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this optimization makes it a bit tricky to understand - Type mapping tests should also be simple enough to extend them or additional test cases in the future - they could be redundant if we are trying to add multiple values like timestamp '2024-01-01 12:34:56.1111

@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging for triage help: @mosabua

@github-actions github-actions bot added the stale label Dec 17, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions bot closed this Jan 7, 2025
@ssheikin ssheikin reopened this Jan 13, 2025
@github-actions github-actions bot removed the stale label Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Praveen2112 Praveen2112 Praveen2112 left review comments

@hashhar hashhar Awaiting requested review from hashhar

Assignees
No one assigned
Labels
cla-signed docs
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ssheikin @Praveen2112