kn: Implement `Instant` using `kotlinx.datetime.Instant` #1201

lauzadis · 2024-12-20T21:08:25Z

Issue #

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ianbotsf · 2024-12-20T22:57:39Z

runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/time/TimestampFormat.kt

-     * ISO-8601/RFC5399 timestamp including fractional seconds at microsecond precision (e.g.,
+     * ISO-8601/RFC3339 timestamp including fractional seconds at microsecond precision (e.g.,
     * "2022-04-25T16:44:13.667307Z")
     *
-     * Prefers RFC5399 when formatting
+     * Prefers RFC3339 when formatting


Comment: Oh jeez, good catch. 🤦‍♂️

ianbotsf · 2024-12-20T23:27:34Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/time/InstantNative.kt

+            // Handle leap seconds (23:59:59)
+            parsed.second = parsed.second?.coerceAtMost(59)


Correctness: This looks like it's discarding leap seconds not handling them. 60 is a valid leap second.

Yes this is what we're currently expecting to happen: https://github.com/smithy-lang/smithy-kotlin/blob/kn-time/runtime/runtime-core/common/test/aws/smithy/kotlin/runtime/time/InstantTest.kt#L57-L58

Do you want to change the logic here?

Almost certainly, yes. Treating x:y:60 and x:y:59 as the same instant is incorrect. The problem gets worse when subsecond precision is considered and timestamps increment like :59.998, :59.999, :59.000, :59.001.

We should verify with other SDK implementations but I believe our existing test (and therefore likely our existing JVM implementation) is flawed.

We spoke offline and agreed to match Java's Instant behavior here, which effectively truncates the leap second 23:59:60 to 23:59:59.

ianbotsf · 2024-12-20T23:30:41Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/time/InstantNative.kt

+            // Handle leap hours (24:00:00)
+            return if (parsed.hour == 24) {
+                parsed.hour = 0
+                Instant(parsed.toInstantUsingOffset() + 1.days)
+            } else {
+                Instant(parsed.toInstantUsingOffset())
+            }


Correctness: What's a leap hour?

Sorry not a leap hour, it's a midnight timestamp

https://github.com/smithy-lang/smithy-kotlin/blob/kn-time/runtime/runtime-core/common/test/aws/smithy/kotlin/runtime/time/InstantTest.kt#L59-L60

OK, I don't think that's a thing with RFC 3339:

From § 5.6:

date-month = 2DIGIT ; 01-12 date-mday = 2DIGIT ; 01-28, 01-29, 01-30, 01-31 based on ; month/year time-hour = 2DIGIT ; 00-23 time-minute = 2DIGIT ; 00-59 time-second = 2DIGIT ; 00-58, 00-59, 00-60 based on leap second ; rules```

Note that time-hour ranges from 0 to 23, not 24.

From § 5.7:

Although ISO 8601 permits the hour to be "24", this profile of ISO 8601 only allows values between "00" and "23" for the hour in order to reduce confusion.

ianbotsf · 2024-12-20T23:33:47Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/time/InstantNative.kt

+    override fun hashCode(): Int {
+        var result = delegate.hashCode()
+        result = 31 * result + epochSeconds.hashCode()
+        result = 31 * result + nanosecondsOfSecond
+        return result
    }


Nit: epochSeconds and nanosecondsOfSecond are derived from delegate. They shouldn't be part of the hash code calculation.

ianbotsf · 2024-12-20T23:36:15Z

runtime/runtime-core/common/test/aws/smithy/kotlin/runtime/time/InstantTest.kt

Question: Because of some of the refactoring, it's a bit tricky for me to see what's functionally changed in these tests. Besides the addition of .fromEpochSeconds(${test.sec}, ${test.ns}) to an assert message, what's changed here?

Nothing else changed, it's just restructured

ianbotsf · 2024-12-20T23:37:05Z

runtime/runtime-core/common/test/aws/smithy/kotlin/runtime/time/InstantTest.kt

                    .format(format)
                val expected = getter(test)
-                assertEquals(expected, actual, "test[$idx]: failed to correctly format Instant as $format")
+                assertEquals(expected, actual, "test[$idx]: failed to correctly format Instant.fromEpochSeconds(${test.sec}, ${test.ns})  as $format")


Nit: Stray space in message (") as")

ianbotsf · 2025-01-07T17:51:31Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/time/InstantNative.kt

-private fun KtInstant.truncateToMicros(): KtInstant = KtInstant.fromEpochSeconds(epochSeconds, nanosecondsOfSecond / 1_000 * 1_000)
+private fun KtInstant.truncateToMicros(): KtInstant = KtInstant.fromEpochSeconds(epochSeconds, nanosecondsOfSecond / 1000 * 1000)


Question: Why did this change? I don't particularly need separators in relatively small numbers like this but, to me, they're harmless in cases like this and very helpful in cases of larger numbers.

No reason besides change of personal preference in this case, I'll add it back

ianbotsf · 2025-01-07T17:58:33Z

runtime/runtime-core/native/src/aws/smithy/kotlin/runtime/time/InstantNative.kt

-    override fun hashCode(): Int {
-        var result = delegate.hashCode()
-        result = 31 * result + epochSeconds.hashCode()
-        result = 31 * result + nanosecondsOfSecond
-        return result
-    }


Correctness: This removes too much from the hash calculation. We don't need the entropy from epochSeconds or nanosecondsOfSecond since they're derived values but we do need the entropy from delegate itself. Otherwise, instances will use the default hashcode implementation which is based on instance memory location and will diverge even for objects which are structurally equal.

…n-time

github-actions · 2025-01-08T15:04:24Z

Affected Artifacts

Changed in size

Artifact	Pull Request (bytes)	Latest Release (bytes)	Delta (bytes)	Delta (percentage)
http-test-jvm.jar	61,633	58,718	2,915	4.96%
crt-util-jvm.jar	21,443	20,954	489	2.33%
runtime-core-jvm.jar	813,449	812,469	980	0.12%
aws-signing-tests-jvm.jar	456,614	456,568	46	0.01%
test-suite-jvm.jar	96,907	97,180	-273	-0.28%

lauzadis added 2 commits December 20, 2024 16:07

Initial commit / tests are passing

5cdf2dd

ktlint

43a9dfb

lauzadis changed the title ~~Initial commit / tests are passing~~ kn: Instant Dec 20, 2024

lauzadis added 2 commits December 20, 2024 16:24

Fix a failing test

4e1aab0

Add Kdocs

72fa04f

lauzadis marked this pull request as ready for review December 20, 2024 21:32

lauzadis requested a review from a team as a code owner December 20, 2024 21:32

lauzadis changed the title ~~kn: Instant~~ kn: Implement Instant using kotlinx.datetime.Instant Dec 20, 2024

ianbotsf requested changes Dec 20, 2024

View reviewed changes

lauzadis added 6 commits January 6, 2025 14:24

Remove hashCode

79b5a9a

Remove support for midnight hours

1171ecb

ktlint

34bb00b

clean up

2ddf062

ktlint

27f9966

CI

e5e739f

ianbotsf requested changes Jan 7, 2025

View reviewed changes

lauzadis added 2 commits January 7, 2025 13:32

Use delegate.hashCode()

b92d4a2

Replace underscore separator

86c1d96

ianbotsf approved these changes Jan 7, 2025

View reviewed changes

Merge branch 'kn-main' of github.com:smithy-lang/smithy-kotlin into k…

04cc6f4

…n-time

lauzadis added the no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly. label Jan 8, 2025

This comment has been minimized.

Sign in to view

0marperez approved these changes Jan 8, 2025

View reviewed changes

lauzadis merged commit 43e39b8 into kn-main Jan 8, 2025
21 of 22 checks passed

lauzadis deleted the kn-time branch January 30, 2025 15:26

		// Handle leap seconds (23:59:59)
		parsed.second = parsed.second?.coerceAtMost(59)

		private fun KtInstant.truncateToMicros(): KtInstant = KtInstant.fromEpochSeconds(epochSeconds, nanosecondsOfSecond / 1_000 * 1_000)
		private fun KtInstant.truncateToMicros(): KtInstant = KtInstant.fromEpochSeconds(epochSeconds, nanosecondsOfSecond / 1000 * 1000)

kn: Implement Instant using kotlinx.datetime.Instant #1201

kn: Implement Instant using kotlinx.datetime.Instant #1201

Uh oh!

Conversation

lauzadis commented Dec 20, 2024

Issue #

Description of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

github-actions bot commented Jan 8, 2025

Affected Artifacts

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kn: Implement `Instant` using `kotlinx.datetime.Instant` #1201

kn: Implement `Instant` using `kotlinx.datetime.Instant` #1201