[IOTDB-17433] Fix timezone offset bug in DateTimeUtils#17439
[IOTDB-17433] Fix timezone offset bug in DateTimeUtils#17439Lexert19 wants to merge 2 commits intoapache:masterfrom
Conversation
6cc5242 to
1ddb460
Compare
There was a problem hiding this comment.
Pull request overview
Fixes incorrect timezone offset resolution in DateTimeUtils for zones with DST by deriving offsets from the provided datetime string rather than the current system time.
Changes:
- Refactors
DateTimeUtils.toZoneOffsetto take(String, ZoneId)and compute the offset for the datetime being parsed. - Updates
convertDatetimeStrToLongoverloads to use the new offset resolution logic. - Adds unit tests covering DST winter/summer offsets, transitions, explicit offsets in strings, and historical LMT offsets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/DateTimeUtils.java | Computes zone offsets from the input datetime string (instead of Instant.now()), and wires it into datetime parsing. |
| iotdb-core/datanode/src/test/java/org/apache/iotdb/db/utils/DateTimeUtilsTest.java | Adds DST/offset-related coverage and adjusts an existing test to avoid using the current time for offsets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Test | ||
| public void testToZoneOffsetJustBeforeSpringDST() { | ||
| ZoneId zoneId = ZoneId.of("Europe/Warsaw"); | ||
| ZoneOffset offset = DateTimeUtils.toZoneOffset("2024-03-31 02:00:00", zoneId); |
There was a problem hiding this comment.
testToZoneOffsetJustBeforeSpringDST uses 2024-03-31 02:00:00 for Europe/Warsaw, but that local time is inside the spring-forward DST gap (the clock jumps from 02:00 to 03:00), so the test name/intent doesn’t match the chosen timestamp and the expected offset is ambiguous. Consider changing this to a valid instant just before the transition (e.g., 01:59:59) to make the assertion deterministic and aligned with the test name.
| ZoneOffset offset = DateTimeUtils.toZoneOffset("2024-03-31 02:00:00", zoneId); | |
| ZoneOffset offset = DateTimeUtils.toZoneOffset("2024-03-31 01:59:59", zoneId); |
| @Test | ||
| public void testToZoneOffsetDuringSpringDSTGap() { | ||
| ZoneId zoneId = ZoneId.of("Europe/Warsaw"); | ||
| ZoneOffset offset = DateTimeUtils.toZoneOffset("2024-03-31 02:30:00", zoneId); | ||
| Assert.assertEquals(ZoneOffset.ofHours(1), offset); | ||
| } |
There was a problem hiding this comment.
testToZoneOffsetDuringSpringDSTGap asserts an offset for 2024-03-31 02:30:00 in Europe/Warsaw, but this local time does not exist due to the DST spring-forward gap. The current expectation relies on how gaps are resolved by ZoneRules and can become brittle across JDK/tzdb changes. It would be more robust to either (a) use ZoneRules#getValidOffsets/transition logic in production code and assert that documented behavior here, or (b) change the test to expect a failure/explicit resolution rather than asserting a specific offset for a nonexistent local time.
|
|
||
| public static ZoneOffset toZoneOffset(ZoneId zoneId) { | ||
| return zoneId.getRules().getOffset(Instant.now()); | ||
| /** Converts string to ZoneOffset. Truncates seconds for HH:mm database compatibility. */ |
There was a problem hiding this comment.
The new toZoneOffset(String, ZoneId) Javadoc says it “Truncates seconds for HH:mm database compatibility”, but the implementation actually truncates offset seconds (i.e., converts the computed ZoneOffset to minute granularity). Consider clarifying the comment to avoid implying that input datetime seconds are truncated, and document how DST gaps/overlaps are resolved (since this method now derives offsets from local date-times).
| /** Converts string to ZoneOffset. Truncates seconds for HH:mm database compatibility. */ | |
| /** | |
| * Converts a date-time string to a {@link ZoneOffset}. | |
| * | |
| * <p>If the input ends with {@code Z}, this method returns {@link ZoneOffset#UTC}. If the | |
| * input contains an explicit {@code \u00b1HH:mm} offset, that offset is returned directly. | |
| * Otherwise, the input is interpreted as a local date-time in the supplied {@code zoneId}, and | |
| * the corresponding zone offset is derived from that local date-time. | |
| * | |
| * <p>For database {@code HH:mm} compatibility, this method truncates the resulting offset to | |
| * minute precision by discarding any offset seconds. It does not truncate the seconds field of | |
| * the input date-time. | |
| * | |
| * <p>When deriving the offset from a local date-time, DST transitions are resolved using {@link | |
| * java.time.zone.ZoneRules#getOffset(LocalDateTime)}: for overlaps, the earlier offset is | |
| * returned; for gaps, the offset before the transition is returned. | |
| */ |
| public static long convertDatetimeStrToLong(String str, ZoneId zoneId) { | ||
| return convertDatetimeStrToLong( | ||
| str, | ||
| toZoneOffset(zoneId), | ||
| toZoneOffset(str, zoneId), | ||
| 0, | ||
| CommonDescriptor.getInstance().getConfig().getTimestampPrecision()); | ||
| } | ||
|
|
||
| public static long convertDatetimeStrToLong( | ||
| String str, ZoneId zoneId, String timestampPrecision) { | ||
| return convertDatetimeStrToLong(str, toZoneOffset(zoneId), 0, timestampPrecision); | ||
| return convertDatetimeStrToLong(str, toZoneOffset(str, zoneId), 0, timestampPrecision); |
There was a problem hiding this comment.
convertDatetimeStrToLong(String, ZoneId) now calls toZoneOffset(str, zoneId), and toZoneOffset reparses the same string via convertDatetimeStrToLong(str, ZoneOffset.UTC, ...) before convertDatetimeStrToLong parses it again with the resolved offset. This introduces an extra full parse for datetime strings without an explicit offset, which could be a hot-path performance regression. If this method is performance-sensitive, consider extracting the local date-time once (or otherwise avoiding the double parse) while still supporting the various input formats.
Description
Refactored DateTimeUtils.toZoneOffset to calculate the timezone offset based on the provided string instead of the current system time (Instant.now()).
Added comprehensive unit tests in DateTimeUtilsTest to cover DST transitions.
Fixes #17433
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR