fix(core): fix hour parsing and formatting in the 1-12 and 1-24 conventions #4506
Conversation
This file contains 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
The 'hh' pattern stands 'Hour in am/pm (1-12)' The original implementation was subtracting 1. As a result it parsed e.g. 05:00 as if it was 04:00, which is clearly wrong. It adds tests comparing 12-hour clock parser/formatter with a refernce SimpleDateFormat from JDK. Fixes #4502
|
there seems to be the same problem with the |
they relied on the old buggy behaviour
also: modulo by a constant is cheap. ver likely better than a 50% mispredition chance
…h_12hour_clock_format
jerrinot
changed the title
jerrinot
changed the title
ideoma
reviewed
May 16, 2024
core/src/test/java/io/questdb/test/std/datetime/millitime/DateFormatCompilerTest.java
Show resolved
Hide resolved
core/src/test/java/io/questdb/test/std/datetime/microtime/TimestampFormatCompilerTest.java
Show resolved
Hide resolved
core/src/test/java/io/questdb/test/std/datetime/millitime/DateFormatCompilerTest.java
Show resolved
Hide resolved
ideoma
approved these changes
May 16, 2024
|
unrelated test failure: #4511 |
[PR Coverage check]😍 pass : 57 / 57 (100.00%) file detail
|
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.
The
hhpattern stands for 'Hour in am/pm (1-12)'. The original parser implementation was subtracting 1 and a formatter was adding 1.As a result, it parsed e.g. 05:00 as if it was 04:00, which is wrong. But when converted back to a string (with the same pattern) it was formatted correctly - displaying
05:00again. Thus the bug was not immediately visible. I assume it was an attempt to convert a 1-based indexing into a 0-based indexing. But clocks are not that simple. The 12-hour system with the 1-12 convention goes like this: 12am (midnight), 1am, 2am, 3am, ..... 11am, 12pm (noon), 1pm, 2pm, ... 11pm and then wraps over to 12am (midnight) of the new day.The same problem was with the
kk-Hour in day (1-24)pattern. It was also subtracting 1 during parsing and adding one when formatting.Implementation notes:
There is little difference between 0-11 and 1-12 schemes. The only difference is that the 1-12 scheme uses
12for noon and midnight whereas the 0-11 scheme uses0for noon and midnight. All other hour values are the same. We use this property to simplify the code parsing: We parse both 0-11 and 1-12 by using the same code. Later, when constructing alongtimestamp, we treat 12 as if it was 0. Regardless of the 0-11 vs 1-12 scheme. One side effect is that the parser is somewhat more relaxed: Even when a parser is configured with the 0-11 it will still accept12as an hour value. I don't think it can cause issues in practice, except for possible compatibility issues if we ever decide to make it stricter again.This change also adds tests comparing both
hhandkkpatterns with a referenceSimpleDateFormatfrom JDK.Fixes #4502