Fix ClickHouseColumn: resolve Fixed/UTC±HH:MM:SS timezone names to correct offset#2877
Open
polyglotAI-bot wants to merge 3 commits into
Open
Fix ClickHouseColumn: resolve Fixed/UTC±HH:MM:SS timezone names to correct offset#2877polyglotAI-bot wants to merge 3 commits into
polyglotAI-bot wants to merge 3 commits into
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 |
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. |
Comment on lines
+3
to
+8
| import java.util.Collections; | ||
| import java.util.LinkedList; | ||
| import java.util.List; | ||
| import java.util.TimeZone; |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 7d48053 — the unused java.util.TimeZone import is removed. You're right that it was unused: the test only references the method ClickHouseColumn.resolveTimeZone(...) and column.getTimeZone(), never the TimeZone type.
One correction for the record: an unused import does not fail javac compilation (the Compile (JDK 8) check was green on the prior commit). Removed regardless as a hygiene cleanup.
Generated by Nerve
Comment on lines
+146
to
+158
| static TimeZone resolveTimeZone(String tzName) { | ||
| if (tzName != null && tzName.startsWith("Fixed/UTC")) { | ||
| String offset = tzName.substring("Fixed/UTC".length()); | ||
| if (!offset.isEmpty()) { | ||
| try { | ||
| return TimeZone.getTimeZone(ZoneOffset.of(offset)); | ||
| } catch (DateTimeException ignored) { | ||
| // fall through to standard resolution | ||
| } | ||
| } | ||
| } | ||
| return TimeZone.getTimeZone(tzName); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Thanks — addressed the documentation part in 7d48053 by taking your "update the Javadoc" option: tzName is now documented as non-null and the return is qualified as "never null for a non-null tzName".
I verified the underlying claim empirically (JDK 17): TimeZone.getTimeZone((String) null) does throw NullPointerException. However, I deliberately did not add a runtime null guard, because:
- Null is unreachable on every call path. All four call sites pass
column.parameters.get(i).replace("'", "").String.replacereturns non-null, and a null list element would already NPE at.replace(...)beforeresolveTimeZoneis entered — so the method is never invoked withnull. - No new/reachable NPE is introduced. The pre-existing code called
TimeZone.getTimeZone(parameters.get(i).replace("'", ""))directly and had identical null behavior; this PR only inserts theFixed/UTCbranch ahead of that same delegating call. - A defensive
null→default guard would change behavior on an unreachable path and risks masking a genuine upstream parsing bug, whichdocs/changes_checklist.md→ "Null handling changed" cautions against ("New null checks do not hide genuine bugs").
So the clarified Javadoc makes the existing non-null contract explicit without altering behavior. Happy to add Objects.requireNonNull(tzName) instead if maintainers would prefer an explicit precondition check.
Generated by Nerve
…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
|
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.