fix(jdbc-v2): preserve DateTime column timezone in getTimestamp - #2991
fix(jdbc-v2): preserve DateTime column timezone in getTimestamp#2991arimu1 wants to merge 1 commit into
Conversation
ResultSet.getTimestamp was converting ZonedDateTime via the caller Calendar (JVM default), reinterpreting wall-clock components and shifting the epoch when the column timezone differed from the Calendar. Convert using the column/server zone already attached by the binary reader so the absolute instant is preserved across columns with different declared timezones. Fixes ClickHouse#2787
|
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 77ee79b. Configure here.
| } | ||
| wasNull = false; | ||
|
|
||
| return DataTypeUtils.toSqlTimestamp(zdt.toLocalDateTime(), cal.getTimeZone()); |
There was a problem hiding this comment.
getTime disagrees with getTimestamp
Medium Severity
getTimestamp now converts DateTime* values using the column zone so epoch millis match the wire instant, but getTime still takes the column wall-clock LocalDateTime and anchors it with the caller Calendar timezone. On the same row, getTimestamp and getTime can disagree whenever the column timezone and Calendar differ.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 77ee79b. Configure here.
There was a problem hiding this comment.
@arimu1
This is very correct comment. Calendar used to reinterpret timestamp internal value to make it work according to spec.
|
Thank you for the contribution! I'm closing the PR. Please feel free to start discussion in the original issue. Thanks! |


Summary
Fixes
ResultSet.getTimestamp/getObject(..., Timestamp.class)ignoring the column timezone forDateTime/DateTime32/DateTime64values (#2787).BinaryStreamReaderalready attaches the column (or server/session) timezone when building the internalZonedDateTime. The JDBC layer was then converting via the callerCalendar/ JVM default, re-interpreting wall-clock components and shifting the returned epoch when that timezone differed from the column's (e.g.DateTime64(3, 'UTC')under a non-UTC JVM, or two columns with different declared timezones).Changes
ResultSetImpl.getTimestamp: convert using the zone on theZonedDateTime(column/server zone), not the callerCalendarJdbcUtils.convert: preserve absolute instant when convertingZonedDateTime/OffsetDateTime/Instant→TimestampDetachedResultSet.getTimestamp: return the already-materialised absoluteTimestampwithout re-shifting wall-clock componentsCHANGELOG.md+docs/features.mdCompatibility
getTimestampfor zonedDateTime*columns now returns the correct epoch millis (matching the wire instant). Callers that previously compensated by passing a column-matchingCalendarstill get the correct value; callers that relied on the incorrect Calendar reinterpretation will see corrected millis.Calendarparameter remains on the API but is not used to re-shift values that already carry timezone information (matches the JDBC javadoc intent for types that store timezone context).Test plan
mvn -pl client-v2 test -Dtest=DataTypeUtilsTests(39 tests, unit)mvn -pl jdbc-v2 test -Dtest=JdbcUtilsTest(16 tests, unit)JDBCDateTimeTests.testGetTimestampPreservesInstantAcrossColumnTimezones(requires ClickHouse)Fixes #2787