Fix ClickHouseColumn: resolve Fixed/UTC±HH:MM:SS timezone names to correct offset - #2877
Merged
Merged
Conversation
…rrect offset TimeZone.getTimeZone(String) does not recognise the synthetic fixed-offset timezone names that ClickHouse emits in column type metadata — names of the form Fixed/UTC±HH:MM:SS (e.g. Fixed/UTC+05:30:00). The JDK silently falls back to GMT for any unrecognised zone ID, so DateTime columns declared with a literal offset stored UTC in ClickHouseColumn.timeZone, shifting every wall-clock value read from those columns by the declared offset. Introduce a package-private resolveTimeZone helper that detects the Fixed/UTC prefix, extracts the offset string, and delegates to ZoneOffset.of() + TimeZone.getTimeZone(ZoneId) — both available since Java 8. All four call sites in ClickHouseColumn.update() now use resolveTimeZone instead of TimeZone.getTimeZone directly. Fixes: #2876
|
|
|
Repository collaborators can run the JMH benchmark suite against this PR by commenting: Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%): Only one benchmark run per PR is active at a time — issuing a new |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect timezone handling for ClickHouse DateTime* columns whose type metadata contains synthetic fixed-offset timezone IDs like Fixed/UTC+05:30:00, ensuring values are rendered using the declared offset instead of silently falling back to GMT/UTC.
Changes:
- Add
ClickHouseColumn.resolveTimeZone(String)to translateFixed/UTC±HH:MM:SSinto a properZoneOffset-backedTimeZone. - Update
ClickHouseColumn.update()datetime parsing paths to useresolveTimeZone(...)instead ofTimeZone.getTimeZone(String). - Add a unit regression test validating fixed-offset resolution and column parsing behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseColumn.java | Introduces resolveTimeZone and routes datetime timezone resolution through it. |
| clickhouse-data/src/test/java/com/clickhouse/data/ClickHouseColumnTest.java | Adds regression test for Fixed/UTC±HH:MM:SS handling across DateTime variants. |
…olution Covers the two new-code lines flagged by SonarCloud on #2877: - resolveTimeZone DateTimeException fallback (out-of-range Fixed/UTC offset, +19:00:00, which exceeds +-18:00 and makes ZoneOffset.of throw) - the helper must degrade gracefully to GMT, not propagate the exception. - the DateTime size>=2 parse site, exercised via DateTime(3, 'Fixed/UTC+05:30:00'), which is handled like DateTime64 and was previously unreached by the test. Test-only change; no production code modified.
Collaborator
Author
|
Pushed a follow-up commit (
Both are test-only; no production code changed. All 40
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Addresses Copilot review on PR #2877: - remove the unused java.util.TimeZone import from ClickHouseColumnTest (the test references resolveTimeZone(...)/getTimeZone() only, never the TimeZone type) - document resolveTimeZone's non-null tzName precondition; no behavior change (null is unreachable from all parse call sites, which pass a non-null replace("'","") result) Fixes: #2876
Contributor
|
@polyglotAI-bot there convert unit tests into integration ones. |
chernser
requested changes
Jul 8, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
- Unit tests should be converted to integration ones
- real fix in com.clickhouse.client.api.data_formats.internal.BinaryStreamReader#readValue(com.clickhouse.data.ClickHouseColumn, java.lang.Class<?>) where we define timezone for the column.
Per maintainer review, move the Fixed/UTC±HH:MM:SS timezone resolution out of the shared clickhouse-data ClickHouseColumn and into client-v2's BinaryStreamReader, where DateTime/DateTime32/DateTime64 values are read and their timezone is obtained. TimeZone.getTimeZone does not recognise ClickHouse's synthetic Fixed/UTC±HH:MM:SS names and silently returns GMT, shifting every value read from such a column by the declared offset; the reader now recovers the offset from the column's declared name via ZoneOffset. Reverts the earlier clickhouse-data change and its unit test; adds an integration test (DataTypeTests) that reads DateTime/DateTime64 columns with Fixed/UTC offsets against a live server and pins the wall-clock, offset and instant, with Asia/Kolkata and UTC contrast cases. Fixes: #2876
Collaborator
Author
|
Thanks @chernser — done in f6146f1.
One limitation worth flagging: |
Contributor
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f6146f1. Configure here.
Contributor
|
@polyglotAI-bot update changelog. Make a short record about fixed issue: |
…c-timezone-resolution
Collaborator
Author
SonarCloud Quality Gate — stale-branch artifact, resolved by merging
|
| Failing condition | Where it actually lives (untouched by this PR) |
|---|---|
| Reliability E | BUG in jdbc-v2/…/StatementImpl.java (try-with-resources) |
| Security C | 4 VULNERABILITY findings in .github/workflows/release.yml |
| Duplication 13.1% | 513 duplicated lines, all in client-v2/…/ClickHouseBinaryFormatReader.java |
| Coverage 0% | jacoco aggregate came up empty under the drift (0 of 67 "new" lines covered, spread across many untouched files) |
I've merged current main into the branch (a merge commit — no rebase/force-push). The diff vs main is now exactly the two files this PR changes (BinaryStreamReader.java + the DataTypeTests integration test), so SonarCloud's new-code set collapses to just those and the spurious findings drop out.
New-code coverage now comes from the testDateTimeWithFixedOffsetTimeZone integration test — I verified locally with the coverage profile that its jacoco aggregate records BinaryStreamReader.resolveTimeZone at 9/11 lines + 9/12 branches (the only misses are defensive fallbacks for malformed / out-of-range Fixed/UTC names, which the server cannot emit). No production or test behavior changed; the integration test still passes against the server after the merge.
Per maintainer review, record the client-v2 DateTime/DateTime64 Fixed/UTC±HH:MM:SS timezone-resolution fix under the 0.10.0-rc1 Bug Fixes section. Fixes: #2876
Collaborator
Author
|
@chernser Done — added a short CHANGELOG entry for #2876 under
|
chernser
approved these changes
Jul 8, 2026
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Fixes #2876.
ClickHouse emits synthetic fixed-offset timezone names of the form
Fixed/UTC±HH:MM:SS(e.g.Fixed/UTC+05:30:00) in column type metadata forDateTime/DateTime64columns declared with a literal offset.TimeZone.getTimeZone(String)does not recognise this format — per its contract it silently returns GMT for any unrecognised zone ID — soClickHouseColumn.update()stored UTC incolumn.timeZone. Downstream,BinaryStreamReaderreads the unix epoch value correctly but renders it in UTC wall-clock rather than the declared +05:30 offset, shifting every timestamp by the column's offset with no exception raised.The fix introduces a package-private
resolveTimeZone(String)helper inClickHouseColumnthat detects theFixed/UTCprefix, extracts the offset string (e.g.+05:30:00), and delegates toZoneOffset.of(offset)+TimeZone.getTimeZone(ZoneId)— both available since Java 8. For all other zone names (IANA names,UTC,GMT, etc.) the helper delegates directly to the existingTimeZone.getTimeZonecall, preserving their behaviour unchanged. All fourTimeZone.getTimeZone(...)call sites in the datetime switch insideupdate()now useresolveTimeZone.Changes
clickhouse-data/src/main/java/com/clickhouse/data/ClickHouseColumn.java: addresolveTimeZone(String)helper; replace fourTimeZone.getTimeZone(...)calls with it; addjava.time.DateTimeExceptionandjava.time.ZoneOffsetimports.clickhouse-data/src/test/java/com/clickhouse/data/ClickHouseColumnTest.java: addtestResolveFixedUtcTimezoneunit test coveringFixed/UTC+05:30:00,Fixed/UTC-08:00:00,Fixed/UTC+00:00:00, all three datetime code paths (DateTime,DateTime64,DateTime32), and contrast cases for IANA names and plainUTC.Test
testResolveFixedUtcTimezoneinClickHouseColumnTest:resolveTimeZone("Fixed/UTC+05:30:00").getRawOffset()equals 19 800 000 ms (+05:30) — fails on the old code whereTimeZone.getTimeZonereturns GMT (offset 0).Fixed/UTC-08:00:00), UTC-zero case (Fixed/UTC+00:00:00), all three column-type code paths, and contrast cases for IANA (Asia/Kolkata) and plainUTC.Full
clickhouse-datasuite: 1657 tests, 0 failures, 0 errors.Pre-PR validation gate
TimeZone.getTimeZone("Fixed/UTC+05:30:00")returns GMT (offset 0) on unpatched JDK; test fails without fix, passes with it.TimeZone.getTimeZonefor unrecognised zone IDs.resolveTimeZonehandles theFixed/UTCprefix viaZoneOffset.of()+TimeZone.getTimeZone(ZoneId).AGENTS.md/CLAUDE.md/docs/ai-review.md— TestNG@Test(groups = {"unit"}), no public API change, nodocs/features.mdupdate required (bug fix, not new feature), nochanges_checklist.mdsection triggered.Note
Medium Risk
Changes how column timezones are resolved for datetime types, which affects read/display correctness for fixed-offset columns; scope is limited to metadata parsing with regression tests and no public API change.
Overview
Fixes incorrect DateTime wall-clock rendering for columns whose type metadata uses ClickHouse’s synthetic
Fixed/UTC±HH:MM:SSzone IDs (e.g.Fixed/UTC+05:30:00). PreviouslyTimeZone.getTimeZonetreated those as unknown and silently used GMT, shifting every value by the declared offset.Adds package-private
resolveTimeZoneonClickHouseColumnto map theFixed/UTCprefix viaZoneOffset.of(with fallback to the old behavior on invalid offsets) and wires it into all four datetime timezone assignments inupdate()(DateTime,DateTime32/Time,DateTime64/Time64).testResolveFixedUtcTimezonecovers helper behavior, column parsing paths, IANA names, and out-of-range fallback.Reviewed by Cursor Bugbot for commit 7d48053. Bugbot is set up for automated code reviews on this repo. Configure here.