-
Notifications
You must be signed in to change notification settings - Fork 128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
korlibs.time parsing millis precision issue #2197
Comments
Hi @ArtRoman thanks for reporting and for the highly detailed explanation with tests. It was really useful. I have been investigating and it seems that klock was trying to incorrectly handle Can you check if this works for you before merging? We can still make adjustments if required. Commit here: f3bf1c2 |
Hello Carlos. Sorry for the late reply. Yes, the issue was in incorrect handling of milliseconds while parsing, because you don't know the required precision for But the issue with val formatMs = DateFormat(".S")
assertEquals(1, formatMs.parseUtc(".001").milliseconds)
assertEquals(10, formatMs.parseUtc(".01").milliseconds) // ❌ parsed as 1 ms
assertEquals(100, formatMs.parseUtc(".1").milliseconds) // ❌ parsed as 1 ms
assertEquals(".1", DateTime.fromUnixMillis(100).toString(formatMs))
assertEquals(".1", formatMs.parseUtc(".1").toString(formatMs)) // ❌ outputs .0 The last example shows the broken idempotence, when the same pairs of operations like parse/stringify changes the content (formatting is OK, issue is only in parsing). Or did you keep this intentionally for compatibility? I see your TODO mark. Both Java Time and kotlinx.datetime doesn't have that flexible ability to use single litter in pattern for various precision, so it will be a breaking change for the library. But it will also unify the behavior with others libraries 😀 I really don't know which solution would be better, especially in the long term. |
Oh I see. In that case I'm okay with breaking compatibility if it was a bug. We could create a feature flag or something to enable the old behaviour, but maybe people is either not using it that way, or getting wrong results, so I think it makes sense to keep it simple and just break compatibility this time, specially with the idempotent point. Also I believe the
Regarding compatibility with other libraries, I guess most people will switch to kotlinx-time at some point if didn't do already. But let's fix that at least here. I also added a As an extra comment: Klock, the former korlibs-time, was initially designed using C# API as reference + some property extensions instead of the Java one as I believed the C# API was much better than the Java one. https://learn.microsoft.com/en-us/dotnet/api/system.datetime?view=net-8.0 . Then it was evolved using people's feedback. So it didn't pretend to be closer to the Java one but the C# one. If the point on this is converging the |
I see
I agree, it will be convenient to use for parsing and formatting, same as with
|
So basically on the JVM it seems to completely ignore the number of So I guess this would fail on the JVM: assertEquals(100, format.parseTime("13:12:30.1").millisecond) // ❌ parsed as 1 So maybe the I'm going to be consistent with the JVM regarding |
@ArtRoman I have implemented here what I believe is the best option for what I have explored with the JVM:
I'll keep that PR open so you can validate if it makes sense or I missed something. |
@ArtRoman how are you doing? I have plans to make a major release soon of the libraries. I would like to include this change on the release. Could you have a look? |
@soywiz hello. Sorry for the delay, I've missed UI notifications here. I'm going to take a look now. It's interesting that original issue of kotlinx-datetime has similar questions about consistency of serialization, with links to Java8 LocalDateTime, FasterXML and Python analogies. It also was a bug in Java8 with parsing millis in custom format. So it's a really complicated issue! 😀 |
So from my perspective from what I investigated, the original Java API is misleading. Milliseconds can be only from 0 to 999. Being able to provide just one or two digit for milliseconds is wrong, and more than 3 misleading. For me the ideal solution in an ideal world would be to provide seconds as a "double" with fraction part, but the problem is that in some locales In the case of the |
Sorry, I didn't get that. Which classes do you mention here for parsing/formatting?
@Test
fun javaLocalDateTimeFormatterTest() {
DateTimeFormatter.ISO_OFFSET_DATE_TIME.testJavaLocalDateTimeMs("2020-01-01T13:12:30.1Z") //100000000 ns = 100 ms
DateTimeFormatter.ISO_OFFSET_DATE_TIME.testJavaLocalDateTimeMs("2020-01-01T13:12:30.01Z") //10000000 ns = 10 ms
DateTimeFormatter.ISO_OFFSET_DATE_TIME.testJavaLocalDateTimeMs("2020-01-01T13:12:30.001Z") //1000000 ns = 1 ms
DateTimeFormatter.ISO_OFFSET_DATE_TIME.testJavaLocalDateTimeMs("2020-01-01T13:12:30.0001Z") //100000 ns = 0,1 ms
// Truncated zeroes while formatting:
assertFailsWith<AssertionError> {
DateTimeFormatter.ISO_OFFSET_DATE_TIME.testJavaLocalDateTimeMs("2020-01-01T13:12:30.10Z") //100000000 ns = 100 ms
//expected:<...020-01-01T13:12:30.1[0]Z> but was:<...020-01-01T13:12:30.1[]Z>
}
assertFailsWith<AssertionError> {
DateTimeFormatter.ISO_OFFSET_DATE_TIME.testJavaLocalDateTimeMs("2020-01-01T13:12:30.100Z") //100000000 ns = 100 ms
//expected:<...020-01-01T13:12:30.1[00]Z> but was:<...020-01-01T13:12:30.1[]Z>
}
assertFailsWith<AssertionError> {
DateTimeFormatter.ISO_OFFSET_DATE_TIME.testJavaLocalDateTimeMs("2020-01-01T13:12:30.1000Z") //100000000 ns = 100 ms
//expected:<...020-01-01T13:12:30.1[000]Z> but was:<...020-01-01T13:12:30.1[]Z>
}
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SX").testJavaLocalDateTimeMs("2020-01-01T13:12:30.1Z")
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSX").testJavaLocalDateTimeMs("2020-01-01T13:12:30.10Z")
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSX").testJavaLocalDateTimeMs("2020-01-01T13:12:30.100Z")
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSX").testJavaLocalDateTimeMs("2020-01-01T13:12:30.1000Z")
}
private fun DateTimeFormatter.testJavaLocalDateTimeMs(dateStr: String) {
val parsed = java.time.LocalDateTime.parse(dateStr, this)
val formatted = this.format(parsed.atOffset(ZoneOffset.UTC))
println("$dateStr parsed as $parsed (${parsed.nano} ns), formatted: $formatted")
assertEquals(dateStr, formatted)
} |
It's all OK with S*, as I've checked. It's a small breaking change if anyone is using custom pattern with SSS values, but it's a good chance to switch to
Here I've found some inconsistency in compare to JVM: @Test
fun testMillisKorlibsNew() {
assertEquals(1, DateFormat.ISO_DATE_TIME_OFFSET.parseUtc("2020-01-01T13:12:30.001Z").milliseconds)
assertEquals(10, DateFormat.ISO_DATE_TIME_OFFSET.parseUtc("2020-01-01T13:12:30.010Z").milliseconds)
assertEquals(100, DateFormat.ISO_DATE_TIME_OFFSET.parseUtc("2020-01-01T13:12:30.100Z").milliseconds)
assertEquals(100, DateFormat.ISO_DATE_TIME_OFFSET.parseUtc("2020-01-01T13:12:30.1Z").milliseconds)
assertEquals(100, DateFormat.ISO_DATE_TIME_OFFSET.parseUtc("2020-01-01T13:12:30.10Z").milliseconds)
assertEquals(100, DateFormat.ISO_DATE_TIME_OFFSET.parseUtc("2020-01-01T13:12:30.100Z").milliseconds)
assertEquals(100, DateFormat.ISO_DATE_TIME_OFFSET.parseUtc("2020-01-01T13:12:30.1000Z").milliseconds)
assertEquals(1, DateFormat("yyyy-MM-dd'T'HH:mm:ss.S*Z").parseUtc("2020-01-01T13:12:30.001Z").milliseconds)
assertEquals(10, DateFormat("yyyy-MM-dd'T'HH:mm:ss.S*Z").parseUtc("2020-01-01T13:12:30.010Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.S*Z").parseUtc("2020-01-01T13:12:30.100Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.S*Z").parseUtc("2020-01-01T13:12:30.1Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.S*Z").parseUtc("2020-01-01T13:12:30.10Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.S*Z").parseUtc("2020-01-01T13:12:30.100Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.S*Z").parseUtc("2020-01-01T13:12:30.1000Z").milliseconds)
assertEquals(1, DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ").parseUtc("2020-01-01T13:12:30.001Z").milliseconds)
assertEquals(10, DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ").parseUtc("2020-01-01T13:12:30.010Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ").parseUtc("2020-01-01T13:12:30.100Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ").parseUtc("2020-01-01T13:12:30.1Z").milliseconds) //❌ S, SS and SSS parses .1 as 1ms
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ").parseUtc("2020-01-01T13:12:30.10Z").milliseconds) //❌ S, SS and SSS parses .10 as 10ms
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ").parseUtc("2020-01-01T13:12:30.100Z").milliseconds)
assertEquals(100, DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSSZ").parseUtc("2020-01-01T13:12:30.1000Z").milliseconds) //❌ expected:<100> but was:<0>
}
DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSX").testJavaLocalDateTimeMs("2020-01-01T13:12:30.1Z")
//'2020-01-01T13:12:30.1Z' could not be parsed at index 20
I think that your way parsing |
This looks like the root of the confusions. Then I guess we can change |
Fixed: korlibs/korlibs@582b37c
Since I have removed
That was the SimplerDateTime misconception. It should be fixed now with plain S, SS...
So I guess comparing to I believe this should be enough. Going to merge it. Feel free to reopen it if you believe it makes sense or if I missed something. |
I can't find any example of using var timespan = new TimeSpan(12345678);
Console.WriteLine(timespan.ToString("g", new CultureInfo("de"))); // 0:00:01,2345678
var datetime = DateTime.UtcNow;
Console.WriteLine(datetime.ToString("o", new CultureInfo("de"))); // 2024-05-14T21:38:57.5422080Z And at last I want to notice multiple formats with the similar semantics: DateFormat.FORMAT2 // DateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSZ")
DateFormat.ISO_DATE_TIME_OFFSET // DateFormat("yyyy-MM-dd'T'HH:mm[:ss[.S]]Z").withOptional()
ISO8601.DATETIME_UTC_COMPLETE_FRACTION // IsoDateTimeFormat("YYYYMMDDThhmmss.sssZ", "YYYY-MM-DDThh:mm:ss.sssZ") The second one, for example, will lose precision due to outputting only one milliseconds digit when using as parameter in |
The whole picture after all the changes I did yesterday:
|
I've found an issue with parsing milliseconds using custom format with optional (
[.S]
).0.1 sec is 100 ms, but parsing as 1 millisecond;
0.10 sec is 100 ms, but parsing as 10 milliseconds;
0.100 sec is 100 ms, parsing as 100 milliseconds, that's OK.
Tested on com.soywiz.korlibs.korio:korio:4.0.10, com.soywiz.korge:korlibs-time:5.3.2 and 5.4.0.
For example, Java Time and kotlinx.datetime same parsing is OK, but I used existing format for each test, because I can't create format from string with variable length for milliseconds:
The text was updated successfully, but these errors were encountered: