[connectors][clock] Support time zone offset for the NOW() connector - #6745
Conversation
|
Is this useful enough? Real timezones change offset for DST. |
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. Offset is pinned in the controller merge, rejected by update_pipeline while storage is in use, and covered end-to-end by both Rust and Python tests. The #[serde(default, skip_serializing_if = "is_zero")] on timezone_offset_ms keeps old checkpoints readable. blp's DST point is real but the docs explicitly frame this as a constant offset that does not track any geographic timezone, which is the right choice for a monotone clock.
|
Yes, we cannot change offsets. The user who requested this claimed that's good enough. |
|
Maybe that's because the user's example uses Shanghai, and China has no DST. |
|
Hopefully it will be applicable to US soon too. |
|
But only for future timestamps, I guess. |
|
@Mrhs121 can you please take a look? If you use the open-source version of the software maybe you can even try this? |
6607283 to
61c3678
Compare
Thanks for implementing this and for the ping. I reviewed the changes, and a fixed immutable +08:00 offset is sufficient for our use case. Our source data contains timezone-naive Asia/Shanghai timestamps, and we do not need DST handling. We currently carry a small internal patch that hard-codes an 8-hour offset when producing the NOW() clock record. This PR is a much more complete solution: the offset is configurable, UTC remains the default, and checkpoint/replay behavior is handled correctly. From both the implementation and use-case perspective, LGTM. Once this is merged, we plan to replace our hard-coded internal patch. |
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
|
I think this needs to be merged in 2 steps, the platform does not know about the new setting, so it cannot run the new python test correctly. So I will disable the test, merge, and then re-enable the test after the platform has been updated. |
Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
mythical-fred
left a comment
There was a problem hiding this comment.
Re-review at b85adbd. Ahead 23/behind 2 since 61c3678; substantively three PR-authored commits:
- b79ba85 timezone-offset feature: byte-identical file list & line counts to the previously approved rebase (adapters/controller.rs +12, transport/clock.rs +297/-32, feldera-types config.rs +15/-1, feldera-types transport/clock.rs +117/-1, examples.rs +1, db/operations/pipeline.rs +12, db/test.rs +19, configuration.mdx +1, datetime.md +23, openapi.json +29, runtime_config.py +2, python test_clock_timezone_offset.py +139). Prior APPROVE stands.
- b59c4d3 docs typos: pure prose (millenium->millennium, WEEK ISO-8601 rewrite, nanosecond digit fix, sql_tsi_second anchor, TIMESTAMPDIFF example, timestamp FLOOR upward->downward, synonim->synonym, tiemUnit->timeUnit). All correct.
- b85adbd disables the python integration test by renaming
test_clock_timezone_offset.py->.py.enableme. Effective (pytest won't collect it) and easy to grep. Non-blocking nit: consider a tracking issue reference in the commit message or a stubtest_clock_timezone_offset_disabled.pywithpytest.skip("re-enable after platform upgrade, see #NNNN")so the disable is discoverable without knowing the.enablemeconvention. Not blocking.
Approving.
Fixes #6710
A new configuration value for the pipeline can be used to specify a constant time-zone offset for the NOW() clock.
This offset is applied to all clock values returned. Note that the offset cannot be changed unless storage is cleared, because the NOW() clock is designed to always be monotonic.
Checklist