[Clickhouse] add DateTime64 type support

[Apidcloud]

Description

This adds support to the missing type DateTime64 from ClickHouse.

Motivation and Context

I noticed DateTime64 is not supported. As mentioned in #20958, DateTime64 supports milliseconds, whereas DateTime doesn't.

Impact

We are currently blocked from using prestodb because every entry we have in our tables has a column createdAt with the type DateTime64. Milliseconds make sense when we are dealing with user/system events.

Test Plan

Tests were added showing the creation of a table with a DateTime64 column (mapped from timestamp in prestodb), insertion of timestamps into a DateTime64 column with 0 or multiple millisecond digits, and finally the selection of them.

Release Notes

Clickhouse Connector Changes
* Add ``DateTime64`` type support. :pr:`24344`

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Apidcloud / name: Luís Fernandes (7a3feea)

[ZacBlanco]

You'll need to add some tests to verify the mapping works correctly.

Based on your PR description, it sounds like you may have been trying to create tables in clickhouse through presto? If so, you won't be able to use the same column type names that clickhouse uses. You'll need to use the mapped type. i.e. DateTime64 should map to timestamp. See TestClickhouseDistributedQueries for example tests.

[Apidcloud]

I’ve based myself on that file and timestamp has the same issue. Invalid column type after I try to do an assertUpdate with a create table. Date works fine. Datetime, datetime64, or timestamp don’t. On 9 Jan 2025, at 23:33, Zac Blanco ***@***.***> wrote:  @ZacBlanco requested changes on this pull request. You'll need to add some tests to verify the mapping works correctly. Based on your PR description, it sounds like you may have been trying to create tables in clickhouse through presto? If so, you won't be able to use the same column type names that clickhouse uses. You'll need to use the mapped type. i.e. DateTime64 should map to timestamp. See TestClickhouseDistributedQueries for example tests. — Reply to this email directly, view it on GitHub<#24344 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA4MM4XPY3IFBUHSYFETN5D2J32LZAVCNFSM6AAAAABU4UGAZSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKNBRGA3TMMRQG4>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[steveburnett]

Thanks for the release note! Minor rephrasing suggestion.

== RELEASE NOTES ==

Clickhouse Connector Changes
* Add ``DateTime64`` type support. :pr:`24344`

[Apidcloud]

Changed the release notes. Today im still trying to add the tests. And while i was able to get the create table and insert to kind of work, the data that appears on clickhouse side is different than when I do it through the UI on :8080.

[Apidcloud]

Somehow in the tests, this timestamp string (insert into t1 VALUES (timestamp '2025-01-08 12:35:59.079')) becomes 1736292959079 (Tuesday, 7 January 2025 23:35:59.079), but when I do it on the UI at :8080 the same timestamp string becomes 1736336159079 (Wednesday, 8 January 2025 11:35:59.079). How come?

[Apidcloud]

Ah, it seems the timezone in tests is totally off. How can i change that? I'm based in Vienna, whereas tests are running in Bahia Mexico lol

[ZacBlanco]

You don't need to change the time zone in the tests - it should be left as is. I would recommend using the presto-cli for local testing rather than the UI. You can set your session timezone by specifying a JVM argument to the CLI. If you want your testing environment to match the tests you can set the JVM parameter -Duser.timezone=America/Bahia_Banderas

[Apidcloud]

I'm trying to convert the hour I want in UTC to the test timezone and use that in the insert statement. I assumed it would transform it to the UTC counterpart, but it went back almost 24h. What am I missing?

String tableT = "t1";

        // Get the current time zone
        String testTimeZone = TimeZone.getDefault().getID();

        ZonedDateTime originalTimestamp = ZonedDateTime.parse("2025-01-08T12:34:56.179Z", DateTimeFormatter.ISO_ZONED_DATE_TIME);
        ZonedDateTime adjustedTimestamp = originalTimestamp.withZoneSameInstant(ZoneId.of(testTimeZone));

        // adjustedTimestampString is correct: 2025-01-08 06:34:56.179"
        String adjustedTimestampString = adjustedTimestamp.format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSS"));

        assertUpdate("CREATE TABLE " + tableT + " (ts timestamp not null)");
        assertUpdate("INSERT INTO " + tableT + " (ts) VALUES (timestamp '" + adjustedTimestampString + "')", 1);
        // After insert it is wrong. 2025-01-07 17:34:56.179 
        // In docker, the timezone is UTC within clickhouse and the entry shows 2025-01-07 17:34:56.179

[Apidcloud]

After further checking, the test presto client session is defined in TestingSession.DEFAULT_TIME_ZONE_KEY as Pacific/Apia (UTC+13). But like this the select query is wrong, am I'm also wondering why. Specifically, why is it 6 hours more than the Pacific/Apia?

@Test
    public void testInsertAndSelectFromDateTimeTables() {
        String tableT = "t1";

        // this would get America/Bahia if we were to use it
        String testTimeZone = TimeZone.getDefault().getID();

        ZonedDateTime originalTimestamp = ZonedDateTime.parse("2025-01-08T12:34:56.179Z", DateTimeFormatter.ISO_ZONED_DATE_TIME);
        // presto client test session is set to Pacific/Apia
        ZonedDateTime adjustedTimestamp = originalTimestamp.withZoneSameInstant(ZoneId.of(TestingSession.DEFAULT_TIME_ZONE_KEY.getId()));

        // Pacific/Apia becomes 2025-01-09 01:34:56.179 (correct)
        // America/Bahia becomes 2025-01-08 06:34:56.179" (correct)
        String adjustedTimestampString = adjustedTimestamp.format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss.SSS"));

        assertUpdate("CREATE TABLE " + tableT + " (ts timestamp not null)");
        assertUpdate("INSERT INTO " + tableT + " (ts) VALUES (timestamp '" + adjustedTimestampString + "')", 1);
        // wrong for America/Bahia. 2025-01-07 17:34:56.179. After insert it's wrong. in docker, the timezone is UTC within clickhouse and the entry shows 2025-01-07 17:34:56.179
        // But correct when using Pacific/Apia! 2025-01-08 12:34:56.179
        
        assertQuery(
                "SELECT * FROM " + tableT + " LIMIT 100",
                "VALUES (timestamp  '2025-01-08T12:34:56.179')");
        // select is correct when using America/Bahia. Actual is exactly 2025-01-08T12:34:56.179
        // But wrong when using Pacific/Apia, even though the clickhouse server is now correct with 2025-01-08 12:34:56.179
        // Actual comes as 2025-01-09T07:34:56.179
    }

Comments in the source code that might hint something:

[Apidcloud]

Finally seems to be working. Can you tell me the process of creating the tar.gz prestodb file available for download, so that we can start using this version tomorrow?

[ZacBlanco]

This is a great improvement to the connector. Mostly just style nits. Mainly, I am concerned about the performance. I think it would be great to implement this without any string parsing. Have you attempted any other solutions?

Also, our clickhouse-jdbc client dependency is quite out of date. We might be able to upgrade it to the latest com.clickhouse:clickhouse-jdbc:0.7.2 which may provide more options for timestamps, though I haven't looked further into it.

[Apidcloud]

Thanks for the prompt review. Already pushed the changes.

I do agree the clickhouse jdbc is really outdated, and I tried clickhouse jdbc 0.3.2 at some point. But the server wouldn't start because of this:

I can also remember some issues creating the tpch tables in TestclickHouseDistributedQueries but can't reproduce it again.

Either way, I would try to update clickhouse jdbc to the new org com.clickhouse 0.7.2 in a subsequent MR, as it would most likely have other problems.

Edit:
Also added a table to the docs

[steveburnett]

Thanks for the doc! Nit of formatting in the header, the table looks great!

[Apidcloud]

Can you retrigger the CI? It seems one of the test suites timed out

Edit: Thanks, it seems to be ok now

[steveburnett]

LGTM! (docs)

Pull updated branch, new local doc build, looks good. Thanks!

[ZacBlanco]

Code LGTM. two doc comments.

[Apidcloud]

Rebased and squashed commits

[steveburnett]

One nit of phrasing. Thanks!

[steveburnett]

LGTM! (docs)

Pull updated branch, new local doc build, looks good. Thanks!

[ZacBlanco]

Thanks for this great work!