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.

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

ClickHouseDateValueParser parse LocalDateTime with wrong Time Zone #623

Closed
fernee opened this issue Apr 13, 2021 · 10 comments
Closed

ClickHouseDateValueParser parse LocalDateTime with wrong Time Zone #623

fernee opened this issue Apr 13, 2021 · 10 comments
Labels
Milestone

Comments

@fernee
Copy link

fernee commented Apr 13, 2021

in the method ru.yandex.clickhouse.response.parser.ClickHouseDateValueParser#dateTimeToLocalDateTime, when columnInfo.getTimeZone() is not utc, the code below will change the time zone to utc, so we got a wrong LocalDateTime instance. what's the purpose of the code below?

https://github.com/ClickHouse/clickhouse-jdbc/blob/549a62f180fbc21d30a6a0dbfed376013669d2f9/clickhouse-jdbc/src/main/java/ru/yandex/clickhouse/response/parser/ClickHouseDateValueParser.java#L44-L54

@zhicwu
Copy link
Contributor

zhicwu commented Apr 13, 2021

LocalDateTime has no concept of time zone, it just provides a zero offset UTC+0. On the other hand, ClickHouse sends DateTime formatted using server / column timezone in TabSeparated format - the string value is actually a ZonedDateTime, so I had to convert it back to UTC.

Above workaround is no longer needed after switching to a binary format like RowBinary, because ClickHouse will send unix timestamp instead of formatted datetime.

@fernee
Copy link
Author

fernee commented Apr 14, 2021

@zhicwu yes, unix timestamp can resolve this problem. and sure, LocalDateTime has no concept of time zone, but it dosen't just provides a zero offset UTC+0. we should decide the time zone, then convert to LocalDateTime. In this case, jdbc client should respect the server/column timezone by default, and has ability to customize the timezone for parse time to LocalDateTime

@zhicwu
Copy link
Contributor

zhicwu commented Apr 14, 2021

@zhicwu yes, unix timestamp can resolve this problem. and sure, LocalDateTime has no concept of time zone, but it dosen't just provides a zero offset UTC+0. we should decide the time zone, then convert to LocalDateTime. In this case, jdbc client should respect the server/column timezone by default, and has ability to customize the timezone for parse time to LocalDateTime

Not quite sure what exactly you need. It sounds like you should use ZonedDateTime not LocalDateTime, as the latter one has nothing to do with timezone and the driver already took care of timezone conversion automatically for you. Why not convert LocalDateTime to ZonedDateTime using whatever timezone you need?

@fernee
Copy link
Author

fernee commented Apr 19, 2021

I think ZonedDateTime is for some cases that across the time zones. In the other hand, LocalDateTime is for cases that only in certain time zone, to simplify the handling of time. UTC just a case of certain time zone, but other time zones should also be supported.

@reedycreek
Copy link

@zhicwu I cannot get the same LocalDateTime value that I inserted by using the driver. It is not consistent between the insert (write) and the query(read). Any suggestion?

@zhicwu
Copy link
Contributor

zhicwu commented Apr 20, 2021

@zhicwu I cannot get the same LocalDateTime value that I inserted by using the driver. It is not consistent between the insert (write) and the query(read). Any suggestion?

Sounds like a bug. What's the timezone settings(server/column/client)? And steps to reproduce the issue?

@zhicwu zhicwu added bug and removed question labels Apr 21, 2021
@zhicwu zhicwu added this to the 0.3.1 release milestone Apr 21, 2021
@zhicwu
Copy link
Contributor

zhicwu commented Apr 21, 2021

This is again related to the TabSeparated format on which the driver relies :<

When you insert a LocalDateTime, the driver will format the object as String, and then pass it to server in TabSeparated format. ClickHouseValueFormatter.formatLocalDateTime(LocalDateTime) didn't consider timezone, especially when 1) server is in non-UTC timezone; or 2) DateTime* column uses non-UTC timezone. The first issue can be fixed by passing server TimeZone to the method, but the second cannot be easily fixed because the driver does not even know what the column type is.

I'll make the following changes to fix this along with other similar issues:

  • use RowBinary format when useRowBinary is set to true - this applies to both read and write(except the former uses RowBinaryWithNamesAndTypes)
  • add client-side prepared statement(activate when useClientPrepStmts is true) for insert - it's not going to work for complex queries but should be good enough for simple inserts

Note: above two options will default to false in 0.3.x for backward compatibility, but they will be removed and enabled by default starting from 0.4.0.

@zhicwu zhicwu modified the milestones: 0.3.1 release, 0.3.2 Release Apr 27, 2021
@freestyle-coding
Copy link

This is again related to the TabSeparated format on which the driver relies :<

When you insert a LocalDateTime, the driver will format the object as String, and then pass it to server in TabSeparated format. ClickHouseValueFormatter.formatLocalDateTime(LocalDateTime) didn't consider timezone, especially when 1) server is in non-UTC timezone; or 2) DateTime* column uses non-UTC timezone. The first issue can be fixed by passing server TimeZone to the method, but the second cannot be easily fixed because the driver does not even know what the column type is.

I'll make the following changes to fix this along with other similar issues:

  • use RowBinary format when useRowBinary is set to true - this applies to both read and write(except the former uses RowBinaryWithNamesAndTypes)
  • add client-side prepared statement(activate when useClientPrepStmts is true) for insert - it's not going to work for complex queries but should be good enough for simple inserts

Note: above two options will default to false in 0.3.x for backward compatibility, but they will be removed and enabled by default starting from 0.4.0.

so is there anything I can do to work around with 0.3.1 version?

@zhicwu
Copy link
Contributor

zhicwu commented Jun 6, 2021

@freestyle-coding, a workaround is to use UInt32 as value for DateTime column, for examples:

...
statement.execute('insert into my_table(..., datetime_column, ...) values(..., 1622938546, ...)');
...
preparedStatement.setLong(2, 1622938546L);
...

@zhicwu zhicwu added the module-jdbc JDBC driver label Oct 6, 2021
@zhicwu
Copy link
Contributor

zhicwu commented Dec 29, 2021

New JDBC driver com.clickhouse.jdbc.ClickHouseDriver now uses RowBinary for serialization/deserialization and has better support for handling DateTime32/DateTime64 and timezone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants