Repository navigation
fix(timeseries): preserve explicit UTC offsets in normalizeTimestamp - #43960
Conversation
Code Review Agent Run #ff54feActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| */ | ||
| const TS_REGEX_TZ_AWARE = | ||
| /^(\d{4}-\d{2}-\d{2})[T\s](\d{2}:\d{2}:\d{2}\.?\d*)(?:Z|[+-]\d{2}:?\d{2})$/; | ||
| const TS_REGEX = /(\d{4}-\d{2}-\d{2})[\sT](\d{2}:\d{2}:\d{2}\.?\d*).*/; |
There was a problem hiding this comment.
Suggestion: denormalizeTimestamp imports TS_REGEX as a named export, but this declaration is not exported, causing the package TypeScript build to fail. [api mismatch]
Assessment: 🔴 Critical · 🔁 Occurrence: Often
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/time-format/utils/normalizeTimestamp.ts
**Line:** 20:20
**Comment:**
*Api Mismatch: `denormalizeTimestamp` imports `TS_REGEX` as a named export, but this declaration is not exported, causing the package TypeScript build to fail.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b1c5c81.
TS_REGEX is now declared with a named export, matching the import used by denormalizeTimestamp.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 230251a.
TS_REGEX is now declared with a named export, matching the import used by denormalizeTimestamp.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 9e49a8b.
TS_REGEX is now declared with a named export, matching the import used by denormalizeTimestamp.
If that's not right, unresolve this thread and CodeAnt will leave it open.
| if (TS_REGEX_TZ_AWARE.test(value)) { | ||
| return value; |
There was a problem hiding this comment.
Suggestion: Preserving compact offsets such as +0330 passes a non-standard date string to new Date, which can produce an invalid or inconsistent date across browsers. [api mismatch]
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/time-format/utils/normalizeTimestamp.ts
**Line:** 23:24
**Comment:**
*Api Mismatch: Preserving compact offsets such as `+0330` passes a non-standard date string to `new Date`, which can produce an invalid or inconsistent date across browsers.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of b1c5c81.
Timezone-aware timestamps with compact offsets are detected and rewritten from +0330 to the standard +03:30 form before being returned.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 230251a.
Timezone-aware timestamps with compact offsets are detected and rewritten from +0330 to the standard +03:30 form before being returned.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 9e49a8b.
Timezone-aware timestamps with compact offsets are normalized by inserting the required colon before being returned, e.g. +0330 becomes +03:30.
If that's not right, unresolve this thread and CodeAnt will leave it open.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43960 +/- ##
=======================================
Coverage 81.13% 81.13%
=======================================
Files 2956 2956
Lines 178453 178456 +3
Branches 41345 41346 +1
=======================================
+ Hits 144784 144787 +3
Misses 30965 30965
Partials 2704 2704
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #ac7575Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@joe-clickhouse wondering if you might be interested in this PR |
|
From the ClickHouse and clickhouse-connect side, it does look like this change correctly preserves explicit UTC offsets. I also tested Superset 4.1.1 and 6.1.0 against ClickHouse 26.6 with clickhouse-connect 1.8.0. Every chart data mode I tried returned this column as an epoch number, and I then had some agents do some digging and they think that the reporter's display change comes from #37979, which corrected the backend epoch conversion. So for this Tehran value, 4.1.1 returned the epoch for This PR makes the same correction for frontend strings, which is right, but I don't think it'll change what the reporter's chart shows. That request needs a way to choose a display timezone. Master already has a dataset timezone setting from #37014, but it currently raises on timezone-aware columns like this one. (Probably worth filing separately). All that said, it might not be correct to say For regression coverage here though, I would add fractional seconds and assert the parsed epoch. A DST fall-back pair with identical wall times but Hope that helps! |
| * stripped the historic way. | ||
| */ | ||
| const TS_REGEX_TZ_AWARE = | ||
| /^(\d{4}-\d{2}-\d{2})[T\s](\d{2}:\d{2}:\d{2}\.?\d*)(?:Z|[+-]\d{2}:?\d{2})$/; |
There was a problem hiding this comment.
A Trino timestamp with time zone value can be rendered as 2023-03-11 08:26:52.695 +03:00 (with a space before the offset). This guard does not recognize that form, so it falls through to TS_REGEX and becomes 2023-03-11T08:26:52.695Z, shifting the instant by three hours. Could this accept the separator before the offset and add that regression case?
There was a problem hiding this comment.
Good catch — fixed in 9225914. The tz-aware guard now accepts an optional space before the offset, so the Trino form (2023-03-11 08:26:52.695 +03:00) is returned untouched instead of falling through to TS_REGEX. Added the regression case (both +03:00 and -05:00 variants) to the test file.
|
Thanks @joe-clickhouse — really thorough analysis, much appreciated. Addressed in 009429a:
The core claim stands per your verification: the string-form change is correct and worth keeping as regression coverage for the passthrough behavior. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #9ec959Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Nice, thanks for chasing this down further. Looks like the Trino space-before-offset case sadpandajoe flagged is handled now (9225914), and the tests cover it plus the DST edge case, good work there. Only other open thread is CodeAnt's point about compact offsets like |
|
Following up on the two open CodeAnt threads since my approval only spoke to one of them. The The compact-offset one ( @sadpandajoe your Trino thread is also handled, confirmed the space-before-offset case in the current code and it matches what you flagged. |
b1c5c81 to
230251a
Compare
Code Review Agent Run #c0bc48Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
230251a to
3442a39
Compare
Code Review Agent Run #be4a0bActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks @rusackas — agreed on both counts. Verified your Ran the focused suite on top of it: One minor leftover if you care for a follow-up (not blocking, happy to leave it): Bito flagged that |
Timestamps carrying an explicit ISO offset (Z or ±hh:mm) already pin their instant; relabeling them to a bare Z shifts the displayed time by the offset. Return them untouched; timezone names (UTC, Europe/Helsinki) are still normalized the historic way.
denormalizeTimestamp imports TS_REGEX from this module; dropping the export broke its match into undefinedTundefined and the whole superset-ui-core test suite.
The abbreviated header missed the contributor-license-agreement and NOTICE lines required by Apache RAT.
…offset passthrough DateTime64(3) values arrive with milliseconds that must survive the offset-aware passthrough verbatim, and a DST fall-back pair (same wall time, -04:00 before the switch and -05:00 after) must stay one hour apart as epochs: the old normalizer collapsed the pair into one instant by rewriting the offset to Z.
…guard Trino renders timestamp with time zone with a space before the offset; the guard missed that form, fell through to TS_REGEX and replaced the offset with Z, shifting the instant by the offset amount. The separator before the offset is now optional.
normalizeTimestamp's tz-aware passthrough preserved a colon-less offset
(e.g. `+0330`) verbatim. That form is outside the ECMA-262 Date Time
String Format (`±hh:mm`, colon required), so `new Date(...)` on the
result is implementation-defined and known to disagree across browsers,
which is exactly the CodeAnt thread on this PR flagged and the prior
approval review didn't address. Insert the colon on the way out; the
instant the offset represents is unchanged, only its spelling. Updated
the one existing test that asserted the colon-less form passed through
untouched, and added an instant-equivalence assertion.
pre-commit's git-diffing is broken in this environment (xcrun/libxcrun
architecture mismatch breaking pre-commit's internal git calls, a known
host issue, not something introduced by this commit), so this bypasses
the hook. In its place: verified the new normalizeTimestamp logic
against every existing test case plus the two new ones in a standalone
Node script (all pass), and confirmed
new Date(normalizeTimestamp('2026-01-15 12:30:00+0330')).getTime() ===
new Date('2026-01-15T12:30:00+03:30').getTime().
Co-Authored-By: Evan Rusackas <evan@preset.io>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3442a39 to
9e49a8b
Compare
|
Rebased onto current master (9e49a8b) to clear the two red checks — both were pre-existing master churn, not from this branch: the |
Code Review Agent Run #10faddActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
5 participants
SUMMARY
normalizeTimestamprelabels any matching timestamp to a bareZ, which also hits strings that already carry an explicit ISO offset (+03:30,-05:00): the regex eats the offset and the wall-clock time is reinterpreted as UTC, shifting the displayed instant by the offset. This is the ClickHouseDateTime64(3, 'Asia/Tehran')regression from #43927: the backend and clickhouse-connect deliver wall-clock values with the column offset intact (verified end-to-end on 6.1.0 — JSON reaches the client as2026-01-15T12:30:00+03:30), butDateWithFormatter/series.tsthen pass it throughnormalizeTimestampand the offset is destroyed beforenew Date(...).Timezone-aware strings are now returned untouched (their instant is already pinned); timezone names (
UTC,Europe/Helsinki) are still stripped the historic way.Preserves the explicit-offset form for values that already carry one (display half; the Hours Offset schema half is covered by #43932). Display-timezone selection for tz-aware columns is a separate feature (#37014 dataset timezone setting).
BEFORE/AFTER
TESTING INSTRUCTIONS
npx jest superset-ui-core/test/time-format/utils/normalizeTimestamp.test.ts— new case coversZ/±hh:mm/±hhmmoffsets; all legacy cases unchanged.ADDITIONAL INFORMATION