Skip to content
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

Add an OffsetDateTime type #90

Closed
cromefire opened this issue Jan 3, 2021 · 70 comments
Closed

Add an OffsetDateTime type #90

cromefire opened this issue Jan 3, 2021 · 70 comments
Labels
formatters Related to parsing and formatting

Comments

@cromefire
Copy link

cromefire commented Jan 3, 2021

it would be quite good if there was a type (maybe let's call it OffsetDateTime to distinguish it from a possible ZonedDateTime) that can represent a LocalDateTime and TimeZone ZoneOffset together. This is especially useful for parsing RFC3339 compatible strings in common code (e.g. OpenAPITools/openapi-generator#7353 (comment)).

It can easily be accomplished without any platform-specific code, I've already drafted a small example with should be enough for most use-cases (but uses some custom parsing logic, which will only work for RFC3339 compatible strings)

Example
package com.example

import kotlinx.datetime.*
import kotlin.jvm.JvmStatic

/**
 * Represents a [LocalDateTime] and the respective [ZoneOffset] of it.
 */
public class OffsetDateTime private constructor(public val dateTime: LocalDateTime, public val offset: ZoneOffset) {
    override fun toString(): String {
        return if (offset.totalSeconds == 0) {
            "${dateTime}Z"
        } else {
            "$dateTime$offset"
        }
    }

    /**
     * Converts the [OffsetDateTime] to an [Instant]. This looses the [ZoneOffset] information, because the date and time
     * is converted to UTC in the process.
     */
    public fun toInstant(): Instant = dateTime.toInstant(offset)

    /**
     * Returns a new [OffsetDateTime] with the given [TimeZone].
     */
    public fun atZoneSameInstant(newTimeZone: TimeZone): OffsetDateTime {
        val instant = dateTime.toInstant(offset)
        val newDateTime = instant.toLocalDateTime(newTimeZone)
        return OffsetDateTime(newDateTime, newTimeZone.offsetAt(instant))
    }

    public companion object {
        private val zoneRegex by lazy {
            Regex("""[+\-][0-9]{2}:[0-9]{2}""")
        }

        /**
         * Parses an [OffsetDateTime] from a RFC3339 compatible string.
         */
        @JvmStatic
        public fun parse(string: String): OffsetDateTime = when {
            string.contains('Z') -> OffsetDateTime(
                LocalDateTime.parse(string.substringBefore('Z')),
                TimeZone.UTC.offsetAt(Instant.fromEpochMilliseconds(0)),
            )
            string.contains('z') -> OffsetDateTime(
                LocalDateTime.parse(string.substringBefore('z')),
                TimeZone.UTC.offsetAt(Instant.fromEpochMilliseconds(0)),
            )
            zoneRegex.matches(string) -> {
                val dateTime = LocalDateTime.parse(string.substring(0, string.length - 6))
                val tz = TimeZone.of(string.substring(string.length - 6))
                val instant = dateTime.toInstant(tz)
                val offset = tz.offsetAt(instant)
                OffsetDateTime(
                    dateTime,
                    offset,
                )
            }
            else -> throw IllegalArgumentException("Date \"$string\" is not RFC3339 compatible")
        }

        /**
         * Creates an [OffsetDateTime] from an [Instant] in a given [TimeZone] ([TimeZone.UTC] by default).
         */
        @JvmStatic
        public fun ofInstant(instant: Instant, offset: TimeZone = TimeZone.UTC): OffsetDateTime = OffsetDateTime(
            instant.toLocalDateTime(offset),
            offset.offsetAt(instant),
        )

        /**
         *
         */
        @JvmStatic
        public fun of(dateTime: LocalDateTime, offset: ZoneOffset): OffsetDateTime = OffsetDateTime(dateTime, offset)
    }
}
@cromefire
Copy link
Author

It would probably be useful to use an ZoneOffset for this, but it cannot be (directly) parsed currently

@cromefire
Copy link
Author

conversion from and to java.time is also easy (using the example):

import kotlinx.datetime.toJavaLocalDateTime
import kotlinx.datetime.toJavaZoneOffset
import kotlinx.datetime.toKotlinLocalDateTime
import kotlinx.datetime.toKotlinZoneOffset

public fun java.time.OffsetDateTime.toKotlinOffsetDateTime(): OffsetDateTime {
    return OffsetDateTime.of(
        toLocalDateTime().toKotlinLocalDateTime(),
        toOffsetTime().offset.toKotlinZoneOffset()
    )
}

public fun OffsetDateTime.toJavaOffsetDateTime(): java.time.OffsetDateTime {
    return java.time.OffsetDateTime.of(dateTime.toJavaLocalDateTime(), offset.toJavaZoneOffset())
}

@ilya-g
Copy link
Member

ilya-g commented Jan 12, 2021

So did I get it right that you need such a type to represent deserialized values of the built-in date-time OpenAPI format?
Why is it important to preserve the offset information in this case? What can happen if the Instant type is used instead?

@cromefire
Copy link
Author

So not only specifically for OpenAPI, but in general to represent RFC3339 Datetimes (which is meant to be the Internet standard).
You can of course use an Instant here, but that means loosing the offset information. I don't have a specific use-case at hand right now, but I'm sure there was a reason RFC3339 was designed this way and I can think of some cases where the offset could be useful (for example if the server already converts it to the user local time as configured in the server or this information could also represent the local time of the server). I don't know if it's smart to use this, but as in this specific case it's often about existing APIs, this could render APIs unusable. You might find some more use-cases in the sources of the RFC.
Also as far as I'm aware Instant also can't parse RFC3339.

@justasm
Copy link

justasm commented Jan 12, 2021

One specific use-case - our API deals with public transport departure times (bus, metro, etc) in various cities. Public transport departure times (like flights) are always displayed in the local city time. The API returns times conforming to RFC3339 such as 2020-07-31T09:16:15+02:00. If these are parsed and represented as an Instant, we need an additional redundant data point - the zone offset - to convert to and display LocalDateTimes to the user.

This does not imply that it is necessary to have a kotlinx.datetime.OffsetDateTime, but at the very least it would be useful to have a parsing function that returns a LocalDateTime and ZoneOffset from an RFC3339 time.

@straight-shoota
Copy link

FWIW: Crystal's equivalent of Instant always carries a location property to designate the time zone instance. Internal represenatation is normalized to UTC, so for example comparison between instances with different locations doesn't require conversion. The time zone information is only used for interaction with a human-readable datetime representation (parsing, stringfigication, calendrical arithmetics).
This has proven to be very practical and enables use cases like the one described here without adding too much complexity, and actually simplifying the API because you don't need to pass a TimeZone instance to every method which operates time-zone aware.

@ilya-g ilya-g changed the title Provide a (Local)DateTime with TimeZone information Instant or LocalDateTime with ZoneOffset to represent RFC3339 format parsing results Jan 13, 2021
@harry248
Copy link

Came here searching for a multiplatform replacement for java.time.ZonedDateTime. It's really surprising that kotlinx-datetime isn't equally able to parse an ISO-8601 and/or a RFC-3339 date string that includes a non-zulu time-zone. Or am I getting it wrong?

@hrach
Copy link

hrach commented Apr 28, 2021

Fixed in recent release: https://github.com/Kotlin/kotlinx-datetime/releases/tag/v0.2.0
Sorry, I mistaken this for another issue.

@harry248
Copy link

Fixed in recent release: https://github.com/Kotlin/kotlinx-datetime/releases/tag/v0.2.0

But parsing it to an instant the time zone is getting lost isn't it?

@dkhalanskyjb
Copy link
Collaborator

The parsed time zone is not returned to the user, yes.

@cromefire
Copy link
Author

Which does result in the loss of information

@harry248
Copy link

Then unfortunately this is not a solution / fix for us, as we have to keep the time zone.

@dkhalanskyjb
Copy link
Collaborator

We do want to support returning the parsed offsets, but the exact implementation is unclear: maybe it's sufficient to just return a Pair, maybe something heavier, like ZonedDateTime, needs to be introduced, or something in-between would be the best choice.

To decide this, we need to consider the specific use cases. @harry248, could you please share yours? @justasm already mentioned that an API they're dealing with returns the instants with a time zone that also indicates how the instant should be displayed to the end-user—this is a good point, thanks!

@harry248
Copy link

harry248 commented May 1, 2021

@dkhalanskyjb It's exactly the same scenario in our case.

@dkhalanskyjb dkhalanskyjb added the formatters Related to parsing and formatting label Jun 16, 2021
@i-walker
Copy link

i-walker commented Aug 23, 2021

My use-case for OffsetDateTime is to parse data from a database. So serialization and deserialization are sufficient. But Ideally we can override them automatically thorugh kotlinx

@fluidsonic
Copy link

fluidsonic commented Aug 25, 2021

I've had one (rare & weak) use case for OffsetDateTime so far, just recently:
I had to parse an ISO 8601 timestamp from a 3rd party API. While I could parse it as Instant I also needed to know the offset in that case because that API doesn't provide a proper time zone. However I've asked them to include it as that would make more sense and be more reliable than using offsets.

That use case is more or less #90 (comment) where also the API isn't providing an ideal data format. It should probably provide a local datetime + time zone instead of datetime merged with an offset.

In general over many different projects there hasn't been any instance where I've needed ZonedDateTime.
In most cases where a TimeZone is needed it's stored and passed around separately anyway.
For example for each insurance policy we store a TimeZone that's used for all LocalDate(Time) instances affecting that insurance. No need for ZonedDateTime here. TimeZone is typically part of a context (e.g. the insurance policy, a calendar entry, a geographical location) and thus not linked directly to individual Date(Time) instances. That was true for all projects so far.

An offset I've never needed except for the very rare use case mentioned initially.

@wkornewald
Copy link
Contributor

Our use-case is to be able to losslessly serialize/deserialize and work with arbitrary future datetimes with zone and DST information like java.time.ZonedDateTime already does:

  1. offset only (e.g. +02:00)
  2. zone id only (e.g. Europe/Berlin)
  3. zone id with a flag for DST vs. standard (e.g. [Europe/Berlin][DST], [Europe/Berlin][ST] or maybe +02:00[Europe/Berlin])

Only (3) can correctly represent a future event/datetime if the zone's offset and DST handling gets changed unexpectedly. Using separate LocalDateTime and TimeZone and dst flags is unnecessarily complicated and error-prone.

Finally, with such a flexible ZonedDateTime maybe there is no need for a separate OffsetDateTime (I never had a need for that at least).

@dkhalanskyjb
Copy link
Collaborator

Hi @wkornewald!

Only (3) can correctly represent a future event/datetime if the zone's offset and DST handling gets changed unexpectedly.

Could you elaborate on this? I don't see why this is true. Consider two cases here:

  • The instant is what's important. For example, we may need to run something exactly 2000 hours later. In this case, this information is encoded as an Instant, and optionally a TimeZone to convert to LocalDateTime for human readability.
  • The clock readings is what's important. For example, something, like a meeting or a train departure, needs to happen on June 12th, 13:30. In this case, the right thing to do is to transfer the LocalDateTime + TimeZone only: it doesn't matter if it's DST or not, what offset it is, etc., the only thing that matters is that the date (which is the ground truth in this case) map correctly to the Instant at that instant.

Am I missing some case here where LocalDateTime + TimeZone + ZoneOffset are all important?

In general, IIRC, this is the first time someone asks that we handle the DST in a specific manner.

@wkornewald
Copy link
Contributor

wkornewald commented Dec 13, 2021

Note that point (3) is just an edge case to be able to represent a precise umanbiguous clock readings time. My main suggestion is to have a ZonedDateTime that at least fulfills (1) and (2), but additionally being able to losslessly represent serialized string-based dates produced by java.time would be nice.

Regarding (3): If you want to trigger an event at an exact point in clock readings time then you have two different interpretations. How do you distinguish the point in time before the DST change vs. the time after the change? 02:30 could mean two points in time.

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Dec 13, 2021

How do you distinguish the point in time before the DST change vs. the time after the change? 02:30 could mean two points in time.

Tricky.

  • If we assume that the DST dates do not suddenly change, we can use Instant to represent this: the translation from Instant to LocalDateTime won't be ambiguous.
  • If we assume that the DST dates may suddenly change and the datetime is important, then we want to adapt to the changes. How do we do that?
    • If some datetime used to be ambiguous, but after the rules are changed, it is no longer ambiguous, then the DST flag should be ignored. Seems fairly obvious: since the datetime is important, we don't want to do anything that won't preserve it.
    • If some datetime used to be unambiguous, but after the rules are changed, it becomes ambiguous, then, yes, we could use the DST flag to choose the correct side of the overlap, but would this do any good? My worry is that this choice would be rather arbitrary. If someone used, say, a date picker to choose 12:15 on a particular day and it happened to be DST, but then the rules changed so that 12:15 occurs twice on that day, does the existing DST flag really say anything about what the user would have wanted in this case? I don't think so.
    • If some datetime used to be ambiguous (for example, 12:15 when the clocks are moved from 13:00 to 12:00), and after the rules are changed, it stays ambiguous (for example, the clocks are moved from 12:30 to 11:30), then, yes, the DST flag would help. Now, this (to me) seems really obscure and not worthy of a special case. I don't really know if such an event ever happened even, and wouldn't be surprised if it hasn't.

@wkornewald
Copy link
Contributor

Imagine a „simple“ calendar app that wants to allow users to specify the DST flag when an event occurs at some ambiguous time. The exact day or time when the switch to DST occurs might change in the future and an Instant can’t deal with that, so the only option would be ZonedDateTime with a nullable DST Boolean (part of TimeZone?).

@wkornewald
Copy link
Contributor

But you're right, this is pretty obscure and indeed maybe not worth it - though then we can't be fully compatible with everything representable by java.time then. Anyway, my main request was having a ZonedDateTime where the zone can be either an id or an offset and I just continued to think of obscure edge cases and then the DST question came up, so I compared how java.time handles that. We're discussing the DST point too much and let's better focus on the core ZonedDateTime aspect. 😄

@dkhalanskyjb
Copy link
Collaborator

Imagine a „simple“ calendar app that wants to allow users to specify the DST flag

Can't really imagine that. I'd expect the calendar app to provide a time picker, which, if the author of the calendar so chose, would have some times repeated.

we can't be fully compatible with everything representable by java.time then

Oh, I think it's not a viable goal at all, given how vast the Java Time API is. We want this library to be simple and discoverable, not all-encompassing.

let's better focus on the core ZonedDateTime aspect

Ok, sure. In your case, since, it looks like, you can choose the format in which to send data, right? Then, I think a good choice depending on the circumstance is either an Instant for representing specific moments (+ an optional TimeZone if the way the datetimes are displayed should also be controlled) or LocalDateTime + TimeZone for representing "the moment when the wall clock in the given time zone displays this". What do you think?

@cromefire
Copy link
Author

cromefire commented Dec 13, 2021

I compared how java.time handles that. We're discussing the DST point too much and let's better focus on the core ZonedDateTime aspect.

I think aligning with java's behavior makes a lot of sense instead of rolling your own and dealing with a lot of inconsistent behavior between different services.

LocalDateTime + TimeZone for representing "the moment when the wall clock in the given time zone displays this".

This seems to basically be how Java implements it and is also how I work around it right now.

@wkornewald
Copy link
Contributor

let's better focus on the core ZonedDateTime aspect

Ok, sure. In your case, since, it looks like, you can choose the format in which to send data, right? Then, I think a good choice depending on the circumstance is either an Instant for representing specific moments (+ an optional TimeZone if the way the datetimes are displayed should also be controlled) or LocalDateTime + TimeZone for representing "the moment when the wall clock in the given time zone displays this". What do you think?

I can't fully choose the format because we have an existing system and already pre-existing official specifications that we have to support.

Actively juggling with LocalDateTime and TimeZone as separate values is definitely not enough.

  • How do you compare dates?
  • How do you parse and format dates?
  • How do you serialize/deserialize? This is almost always a single field in the serialized data (e.g. JSON).
  • How do you make sure no mistakes happen because people forget to take the TimeZone into account in the whole codebase with tons of developers of varying experience?

We had to build our own ZonedDateTime to deal with this and I'm pretty sure everyone else is doing the same thing because this is a very common use-case. Other libraries already provide it out of the box and I think this library should do it, too.

@wkornewald
Copy link
Contributor

If you want to keep the API as small as possible then here’s some provocative food for thought: If you had ZonedDateTime instead of Instant maybe people wouldn’t be missing Instant because ZonedDateTime in UTC handles that use-case already.

The question is where you want to make distinctions explicit via types and maybe where you want to provide common base types to allow abstracting the distinction away.

@cromefire
Copy link
Author

If you want to keep the API as small as possible then here’s some provocative food for thought: If you had ZonedDateTime instead of Instant maybe people wouldn’t be missing Instant because ZonedDateTime in UTC handles that use-case already.

The question is where you want to make distinctions explicit via types and maybe where you want to provide common base types to allow abstracting the distinction away.

The use-case for instant is different: It should be use to record past events that may not change (e.g. the Unix timestamp cannot change) while ZonedDateTimes can (e.g. if laws change), which is why they should be used to record things like future appointments, so maybe not impossible to use a common type, you have to be extra careful to not break those functionalities.

@cromefire
Copy link
Author

cromefire commented Dec 14, 2021

Here are the two use-cases, I also mixed those, so we should probably split it to clear it up:

OffsetDateTime(?) (this issue)

Standards

Characteristics

LocalDateTime (offset from UTC)/Instant with a fixed Offset, no time zone identifier allowed (which would be thing that could change, offsets can't).

Good at

  • Representing fixed moments relative to now/semi-absolute
  • Basically everything an Instant is good at with an added offset
  • Comparing them with each other
  • Establishing the order of events
    • Method of comparison has to be clearly and explicitly communicated to the developer

Bad at

  • Some kinds of future dates like calendar dates, that have to change with a named time zone
  • Doing calculations

Problems

  • Leap seconds?
  • Clearly communicate comparisons.

ZonedDateTime

Standards

Probably exist, I don't know any off my head

Characteristics

LocalDateTime with a named TimeZone (these can change) (or LocalDateTime (UTC)/Instant with a fixed Offset?).

Good at

  • Representing things like a user would expect in a calendar (that change change with laws that change).

Bad at

  • Representing an absolute point of time
  • Being unambiguous, they may require rules/interpretation
  • Comparisons
  • Establishing the order of events (Instants) with its natural comparable order (if it would have any)

Problems

  • Same as OffsetDateTime
  • Time changes (DST / no-DST)
  • Changes of laws

@cromefire cromefire changed the title Instant or LocalDateTime with ZoneOffset to represent RFC3339 format parsing results OffsetDateTime Dec 14, 2021
@wkornewald
Copy link
Contributor

@ilya-g I always tried to explicitly say that the ZonedDateTime would handle offset and ID dynamically based on the amount of information available. And depending on how precise we want to represent time we could also still consider even a combination of both offset and ID at the same time.

Basically, what I’d like to see is an object that can precisely identify the local and global time at the same time and adapt to the precision that is available.

@cromefire cromefire changed the title OffsetDateTime Add an OffsetDateTime type Dec 14, 2021
@ilya-g
Copy link
Member

ilya-g commented Dec 14, 2021

@cromefire To add to above, OffsetDateTime, bad at:

  • establishing the order of events (Instants) with its natural comparable order (if it would have any)
  • arithmetic operations between several OffsetDateTimes: usually such operations need a single time zone or offset, and several OffsetDateTimes provide several sources of what can be considered a time zone information.

@cromefire
Copy link
Author

establishing the order of events (Instants) with its natural comparable order (if it would have any)

I think OffsetDateTimes are fine with that (actually good), because they have a static offset so they are unambiguous. That's a Problem for ZonedDateTimes though.

arithmetic operations between several OffsetDateTimes: usually such operations need a single time zone or offset, and several OffsetDateTimes provide several sources of what can be considered a time zone information.

I think comparative operations (<, >, ==, in-rage/between) are fine, but calculating ones (-, +) are a not a good idea.

@ilya-g
Copy link
Member

ilya-g commented Dec 14, 2021

comparative operations (<, >, ==, in-rage/between) are fine

@cromefire Consider a range 2020-01-01T00:00+02 .. 2020-01-01T01:00+02. Does the value 2020-01-01T01:00+03 which represents the same instant as the start of the range lies in this range?
Can you tell the same about the value 2020-01-01T00:00+02 being in the range 2020-01-01T01:00+03 .. 2020-01-01T02:00+03?

The natural order of OffsetDateTime breaks an intuitive expectation that the result of comparison of two OffsetDateTimes should be the same as the result of comparison of Instants they correspond to. That's why in Java, OffsetDateTime additionally has functions isBefore/isAfter/isEqual. Having the similar ones together with the <, >, == operators in Kotlin may be quite confusing.

@cromefire
Copy link
Author

cromefire commented Dec 14, 2021

comparative operations (<, >, ==, in-rage/between) are fine

@cromefire Consider a range 2020-01-01T00:00+02 .. 2020-01-01T01:00+02. Does the value 2020-01-01T01:00+03 which represents the same instant as the start of the range lies in this range? Can you tell the same about the value 2020-01-01T00:00+02 being in the range 2020-01-01T01:00+03 .. 2020-01-01T02:00+03?

I would say no for the first. And yes for the second (assuming inclusive). (all assuming I calculated correctly in my head and basically just comparing the absolute points in time (e.g. UTC / Instant))

The natural order of OffsetDateTime breaks an intuitive expectation that the result of comparison of two OffsetDateTimes should be the same as the result of comparison of Instants they correspond to. That's why in Java, OffsetDateTime additionally has functions isBefore/isAfter/isEqual. Having the similar ones together with the <, >, == operators in Kotlin may be quite confusing.

Well I could certainly agree with having having explicit methods instead of operators, might make more sense. So I think they are good for representing a series of events in a timeline, but yes I think we could put in explicit methods instead of doing something implicit.

@wkornewald
Copy link
Contributor

wkornewald commented Dec 15, 2021

So do we have a preference already? Maybe I should summarize the points again because after @cromefire's summary you both made some additional important points.

Support in practice

Depending on the database, API and official standards you might have to work with either.

  • Offset seems to be more popular with APIs and standards.
  • For databases it's more mixed.
    • I don't have the impression that PostgreSQL's zone abbreviations are 100% equal to offets, so it seems to prefer zone IDs or maybe the combined abbreviation+offset+DST representation (that's two kinds of offsets combined? plus a DST flag?).
    • Elasticsearch can represent almost anything, including the offset + zone id combination (+01:00[Europe/Paris]).
    • MySQL seems to only support offsets.

Of course at the database level, depending on your DB choice, you could use two separate fields if the level of precision is important. With third-party APIs and official standards you have no choice, so this means offsets most of the time.

Precision and correctness / lack of ambiguity:

This is mostly about mapping 1:1 to Instant.

  • Offsets work pretty well for the past, but are error-prone for the future.
  • Zone IDs work okay-ish for the future and past. They're ambiguous during DST transitions, but outside of that there is no ambiguitiy and the zone id has a well-defined offset for every point in time, so they can be mapped 1:1 to Instant (except for a 1h DST winter->summer overlap every year).

Neither is 100% precise and unambiguous all of the time.

Only the combination of offset + zone id (+02:00[Europe/Paris]) could be relatively precise and unambiguous, but it's not always supported. /rant Though, somehow a DST offset, like Europe/Paris+1 for summer time and Europe/Paris+0 for winter time, instead of a UTC-based offset (+02:00[Europe/Paris] for summer time) feels more correct and even less ambiguous (e.g. when the country moves its whole base UTC offset by one hour exactly during the winter -> summer DST transition period).

Comparisons and arithmetic

Here it's not always enough to just map 1:1 to Instant. You need to know the time zone rules.

  • Comparisons: Both work well, but offets win because there are no ambiguities at all.
  • Difference calculations (i.e. duration between two datetimes): both work well, but offsets win again.
  • Adding a duration to a date: Zone IDs work pretty well. Offsets fail because they can't deal with DST-caused offset changes (like @cromefire also noted). But in many cases people use this in combination with comparisons (now > createdOn + 1 hour) in which case DST doesn't matter. So, this is mostly important in reminder and calendar applications where you need a "repeats every ..." feature for events happening within a specific zone ID (not offset).

What's the solution?

This is my personal wish list:

  • Flexibility in the precision and format. Based on what the backend API and/or official spec and/or database allows, any of these can be used:
    • Offset: +02:00
    • Zone ID: Europe/Paris
    • DST shift (or UTC offset?) + Zone ID: Europe/Paris+1 (summer time) vs. Europe/Paris+0 (winter time) and/or +02:00[Europe/Paris] (UTC offset mostly for better compatibility with certain databases and APIs)
  • Arithmetic works for all of those, but if you want correct DST handling you must use offset + zone id or at least zone id (and risk ambiguitity in rare cases).
  • Comparisons are always exactly based on the corresponding Instant.
  • Serialization and formatting/parsing allow lossless conversion of the flexible format (if you have only an offset it will serialize to that). Alternatively, specific formats can be chosen (e.g. ISO8601 or MM/dd/yy etc.).
  • Maybe in a separate i18n lib and outside of the scope of this lib: Formatting/parsing with common language-specific translations like SHORT, MEDIUM, SEMILONG, LONG, etc.

Overall, this would still make ZonedDateTime just a relatively simple wrapper around LocalDateTime + mandatory TimeZone (= RegionTimeZone or FixedOffsetTimeZone) + optional DST offset.

@cromefire
Copy link
Author

cromefire commented Dec 15, 2021

Only the combination of offset + zone id (+02:00[Europe/Paris]) could be precise and pretty unambiguous, but it's not always supported.

This makes basically nothing better, because now you have to choose which one to use if they conflict, which isn't really that much better... The only thing it would help with is having just a normal offset and using the timezones just to display the timezones to the user, but I don't think one would really need a type for that. There's also no standard I know of that uses this.

Offsets fail because they can't deal with DST-caused offset changes

Well because an offset isn't a time zone and isn't trying to be one it doesn't know DST by design. There might even be multiple time zones per offset, it's an irreversible conversion. It a pretty severe limitation of the format.

To proceed in general I'd recommend fast-tracking OffsetDateTime (even if it's just a lighter weight implementation for now, features can always be added), because I think it's pretty straight forward now and then discussing ZonedDateTime which is infinitely more complex. If no one is interested I can probably do I a PR for OffsetDateTime in January, if I can keep it in mind until then.

@wkornewald
Copy link
Contributor

wkornewald commented Dec 15, 2021

Only the combination of offset + zone id (+02:00[Europe/Paris]) could be precise and pretty unambiguous, but it's not always supported.

This makes basically nothing better, because now you have to choose which one to use if they conflict, which isn't really that much better... The only thing it would help with is having just a normal offset and using the timezones just to display the timezones to the user, but I don't think one would really need a type for that. There's also no standard I know of that uses this.

A DST offset instead of UTC offset should be able to solve this, though. In case of an invalid DST offset you just fall back to the only possible time at that point.

Offsets fail because they can't deal with DST-caused offset changes

Well because an offset isn't a time zone and isn't trying to be one it doesn't know DST by design. There might even be multiple time zones per offset, it's an irreversible conversion. It a pretty severe limitation of the format.

To proceed in general I'd recommend fast-tracking OffsetDateTime (even if it's just a lighter weight implementation for now, features can always be added), because I think it's pretty straight forward now and then discussing ZonedDateTime which is infinitely more complex. If no one is interested I can probably do I a PR in January, if I can remember it until then.

Then I'd rather suggest a ZonedDateTime that takes a TimeZone (i.e. either RegionTimeZone or FixedOffsetTimeZone). This way you're at least flexible and you can represent the future correctly. And this also allows to still add an optional DST or UTC offset at a later point without breaking anything or introducing yet another DateTime class.

@cromefire
Copy link
Author

cromefire commented Dec 15, 2021

Then I'd rather suggest a ZonedDateTime that takes a TimeZone (i.e. either RegionTimeZone or FixedOffsetTimeZone). This way you're at least flexible and you can represent the future correctly. And this also allows to still add an optional DST or UTC offset at a later point without breaking anything or introducing yet another DateTime class.

Yes, that's how most implementations of something like that seem to work, I man supporting both types isn't much more complicated than RegionTimeZone I suppose.

But let's pause this here and implement OffsetDateTime first and then continue with ZonedDateTime.

@wkornewald
Copy link
Contributor

Yes, that's how most implementations of something like that seem to work, I man supporting both types isn't much more complicated than RegionTimeZone I suppose.

Mostly the parse() and toString() methods and arithmetic with durations need special handling based on RegionTimeZone vs. FixedOffsetTimeZone. I think the whole rest of the class could be identical for both cases.

@wkornewald
Copy link
Contributor

wkornewald commented Dec 15, 2021

Then I'd rather suggest a ZonedDateTime that takes a TimeZone (i.e. either RegionTimeZone or FixedOffsetTimeZone). This way you're at least flexible and you can represent the future correctly. And this also allows to still add an optional DST or UTC offset at a later point without breaking anything or introducing yet another DateTime class.

Yes, that's how most implementations of something like that seem to work, I man supporting both types isn't much more complicated than RegionTimeZone I suppose.

But let's pause this here and implement OffsetDateTime first and then continue with ZonedDateTime.

But why do you need OffsetDateTime if you can represent them with an almost equally simple ZonedDateTime? I mean, why should we have multiple classes which only confuse other developers?

@cromefire
Copy link
Author

Because it's a much more (on purpose) constrained and a more simple class. I mean why do we want UInt when we have Int? It's artificially constrained and simpler, and that makes life easier. Also see my list above. It can provide certain guarantees that ZonedDateTime can't.

@wkornewald
Copy link
Contributor

Because it's a much more (on purpose) constrained and a more simple class. I mean why do we want UInt when we have Int? It's artificially constrained and simpler, and that makes life easier. Also see my list above. It can provide certain guarantees that ZonedDateTime can't.

Hmm maybe we could later introduce a sealed interface/class for the TimeZone base type and have the two specific types derive from that, so if you parse a random string you work with the base type but if you want to enforce an offset or zone id you can work with the subtypes.

@cromefire
Copy link
Author

So basically you'd want the java Temporal thing that is like a real mess IMO it's so generalized that it's actually pretty useless again and not very indicative of what's going on.

@wkornewald
Copy link
Contributor

wkornewald commented Dec 15, 2021

No I want a ZonedDateTime (with subtypes OffsetDateTime and RegionDateTime - sharing like 90% of the API) that can contain either an offset or a zone id so I can process an arbitrary input (imagine the JSON data can have either an offset or zone id) which gets converted to the more specific subtype (which I could match with a when expression or just keep using the base type), then work with that and format it back to the original input format without any loss. And if I explicitly want to only allow an offset I instead use the subtype‘s parser which only allows offset.

@cromefire
Copy link
Author

cromefire commented Dec 15, 2021

sharing like 90% of the API

Well that's what I meant, I don't think they will share much API if you don't build something super generic. You should use a toOffsetDateTime() in that case. ZonedDateTime will most like likely have methods that don't make a lot of sense on OffsetDateTime, and vice versa.

IMO it's like having LocalTime be a specific type of LocalDateTime. They are related in a way (like most DateTime types), but not quite that similar.

@wkornewald
Copy link
Contributor

wkornewald commented Dec 15, 2021

Temporal is much more arbitrary. No matter if you use an OffsetDateTime or RegionDateTime, the base type ZonedDateTime is a LocalDateTime with offset information (even if it's a zone id based RegionDateTime you can get the offset). This is a lot of information which means you can safely

  • of course parse/format and serialize/deserialize which is useful on its own => no loss of information
  • calculate meaningful (to humans, not mathematicians) durations between two ZonedDateTimes (x - y) => "x hours ago"
  • compare (x > y) => sort the data
  • compare even under arithmetic (x > y + 12 hours) =>check if a limit has been reached

There are also minimum guarantees:

  • if you only use it for the past it reliably represents an Instant (so it's at least as good as OffsetDateTime for the createdOn/modifiedOn use-case)
  • adding a duration behaves at least as precisely as OffsetDateTime (it will not handle DST and future zone changes)

In practice you could say that it's very useful to humans and it's also the way humans would think about these date strings if they just read them.

Temporal is much worse because you don't even know if it's a date or time or instant. That's too arbitrary.

And I can tell from experience that sometimes date fields can have even more precision differences and in one object contain just the year while another has a zoned date time for the same field (or year-month or date etc.) and we have to deal with that somehow and do comparisons and ordering and human-friendly formatting. Still the result is pretty useful to humans as long as you don't mix in a time-only value (which is why Temporal is useless). As long as you build an information hierarchy which strictly adds extra information at each step you can do useful stuff at each information level.

@dkhalanskyjb
Copy link
Collaborator

Also Instant and TimeZone doesn't work

This argument can be used to show that ZonedDateTime also won't work, because it will also adapt its LocalDateTime part. So, Pair<Instant, TimeZone> wins this round: in the case of the pair, the behavior is clear and predictable.

java.time.ZonedDateTime.parse("2021-12-15T10:38:25.793402647+10:00[Europe/Moscow]").toString() ==
    "2021-12-15T03:38:25.793402647+03:00[Europe/Moscow]"

rules for validating the European Union's COVID vaccination certificates:

These can be parsed already with Instant, only the offset will be lost. Is it important in this case?

Well ZonedDateTimes may not even refer to an unambiguous point of time. See the time change. If you need to refer to an exact point of time you need a Instant + TimeZone, which would be different from ZonedDateTime.

When is Instant + TimeZone not equivalent to a ZonedDateTime?


I carefully read all of the above and noticed some things that (I think) are technically incorrect. Let me try to also summarize the state of things.

ZonedDateTime

As implemented in Java, (almost always) equivalent to Instant + TimeZone.

  • (Almost always) automatically adapts LocalDateTime so that the Instant + TimeZone pair is represented correctly.
  • (Almost always) adapts LocalDateTime if it happens to fall into a time gap.

I say "almost always" because is possible to construct a ZonedDateTime that stores arbitrary LocalDateTime, TimeZone, and offset, using ZonedDateTime.ofLenient. I propose that we ignore these malformed things for now, at least until a use case comes up.

So, for ZonedDateTime, Instant and TimeZone work as the source of truth, from which it then recalculates LocalDateTime and the offset as needed.

Arithmetic on ZonedDateTime works the same way the arithmetic on Instant does, but without specifying a time zone, as it comes bundled.

In effect, ZonedDateTime is Pair<Instant, TimeZone>, but

  • With some additional behavior like comparisons also taking LocalDateTime corresponding to the instant into account. Is there evidence that we do need that additional behavior?
  • With more streamlined arithmetic due to not requiring specifying a time zone explicitly. The fact that two parameters often go together into functions is not enough to throw them into one object

We've established only one use case for ZonedDateTime:

  • Parsing/formatting for interoperability with the APIs that also include the time zone in the output.
  • I thought that LocalDateTime + TimeZone is a valid use case, but after a night of sleep, I don't anymore. If we assume that the zone rules may change, then the ZonedDateTime object will be invalidated and recreate its LocalDateTime part, losing some information. If we assume that the rules will not change, OffsetDateTime is also fine for representing this. So, the specific behavior of ZonedDateTime, it seems, is never useful for this.

Good sides of ZonedDateTime:

  • The use cases will be covered. Maybe there's a way to cover them without introducing ZonedDateTime.
  • That's about it?

Issues with introducing ZonedDateTime:

  • Unclear what to do with broken objects, where LocalDateTime + UtcOffset + TimeZone can't be validly combined.
  • Baroque comparisons.
  • Only as portable as the TimeZone object is.
  • Duplicates the functionality of Instant, which will lead to confusion about which one to use.
  • Error-prone when representing a LocalDateTime in the future in an implicit time zone, due to possible rules change.

OffsetDateTime

Equivalent to LocalDateTime + UtcOffset. This information is also enough to recreate an Instant, so, in effect, this is also equivalent to Instant + UtcOffset.

Use cases:

  • Parsing/formatting for interoperability with the APIs for which the offset is a significant piece of information on its own (so Instant's parsing can't be used).
  • If the offset is already known, representing the combined pieces of information:
    • The LocalDateTime somewhere else;
    • The approximate state of the day-night cycle somewhere else;
    • The absolute moment.

Good sides:

  • Portable.
  • No ambiguity.
  • Mostly simple and predictable.

Issues:

  • Again, comparisons. LocalDateTime and UtcOffset don't seem to meaningfully combine here any better in this regard than they would in a Pair<Instant, UtcOffset>.
  • Error-prone when representing a LocalDateTime in the future in an implicit time zone, due to possible rules change.

To summarize: yes, we established that there are some strings that include the time zone that we want to be able to parse; we also know that there are some strings that represent Instant, but also place meaning in the offsets themselves. I propose that we no longer reiterate this, but instead, concentrate on finding some use cases for ZonedDateTime and OffsetDateTime that are not just parsing/formatting or "an Instant, but also a TimeZone/ZoneOffset because it's convenient for them to be sent together".

It's interesting that neither ZonedDateTime nor OffsetDateTime is fully suitable for representing a future event with the ground truth being a LocalDateTime in a specific time zone. Looks like some form of a pair of LocalDateTime and TimeZone may be even more useful than a proper ZonedDateTime.

@wkornewald
Copy link
Contributor

wkornewald commented Dec 15, 2021

rules for validating the European Union's COVID vaccination certificates:

These can be parsed already with Instant, only the offset will be lost. Is it important in this case?

We deserialize the JSON from the backend and serialize it to the DB and I'd personally prefer to always store 100% exactly the original information instead of Instant which means loss of information. In a PR review at least I'd mark the use of Instant as future bug potential - even if right now it might not cause any problems. The future can bring crazy new requirements.

I'd like to emphasize that unintended loss of information is a VERY BAD thing and causes very difficult bugs or impossible problems later. No matter in which context, you need to be careful about keeping the original information content.

Well ZonedDateTimes may not even refer to an unambiguous point of time. See the time change. If you need to refer to an exact point of time you need a Instant + TimeZone, which would be different from ZonedDateTime.

When is Instant + TimeZone not equivalent to a ZonedDateTime?

Just a nitpick: if the time zone DB can get refreshed while the process is running you need LocalDateTime + TimeZone because otherwise you lose stability / reproducibility under serialization.

I carefully read all of the above and noticed some things that (I think) are technically incorrect. Let me try to also summarize the state of things.

ZonedDateTime

As implemented in Java, (almost always) equivalent to Instant + TimeZone.

  • (Almost always) automatically adapts LocalDateTime so that the Instant + TimeZone pair is represented correctly.

  • (Almost always) adapts LocalDateTime if it happens to fall into a time gap.

Do you have an example? My impression is that it adapts the Instant to ensure that the LocalDateTime+TimeZone is stable. Even in the ZonedDateTime source code it's private final LocalDateTime dateTime; and the documentation says:

"""
A ZonedDateTime holds state equivalent to three separate objects, a LocalDateTime, a ZoneId and the resolved ZoneOffset. The offset and local date-time are used to define an instant when necessary. The zone ID is used to obtain the rules for how and when the offset changes. The offset cannot be freely set, as the zone controls which offsets are valid.
"""

To be even more precise, it's actually LocalDateTime+Offset or LocalDateTime+ZoneId depending on what input you provide. So Java's ZonedDateTime is a supertype of, let's call them, OffsetDateTime and RegionDateTime (zone id).

In effect, ZonedDateTime is Pair<Instant, TimeZone>, but

  • With some additional behavior like comparisons also taking LocalDateTime corresponding to the instant into account. Is there evidence that we do need that additional behavior?

I actually dislike that behavior. From an end-user point of view the Instant is what matters for comparisons and ordering.

For special sorting use-cases one can still construct an artificial ordering, but I don't remember ever needing this.

  • With more streamlined arithmetic due to not requiring specifying a time zone explicitly. The fact that two parameters often go together into functions is not enough to throw them into one object

The same argument can be made for date + time. Still we do it because otherwise things become messy (how do you even serialize into a single field if they're separate?).

  • I thought that LocalDateTime + TimeZone is a valid use case, but after a night of sleep, I don't anymore. If we assume that the zone rules may change, then the ZonedDateTime object will be invalidated and recreate its LocalDateTime part, losing some information. If we assume that the rules will not change, OffsetDateTime is also fine for representing this. So, the specific behavior of ZonedDateTime, it seems, is never useful for this.

That can be exactly what you want because people in the affected country keep thinking in LocalDateTime, not in Instant. If a country shifts its base zone offset (which does happen) or moves the DST switch around then your calendar events (meetings, concerts, etc.) should stay unmodified in LocalDateTime and auto-correct the underlying Instant because people in that country keep going to work at the same LocalDateTime and even airports might adapt their active hours (some disallow flights after midnight for example). That's why Java's ZonedDateTime is not Instant+TimeZone, but LocalDateTime+TimeZone.

For the past a ZonedDateTime can be mapped 1:1 to Instant+TimeZone because zone information should never change for the past. ZonedDateTime additionally handles the future more correctly. That's the difference.

Issues with introducing ZonedDateTime:

  • Unclear what to do with broken objects, where LocalDateTime + UtcOffset + TimeZone can't be validly combined.

  • Baroque comparisons.

  • Only as portable as the TimeZone object is.

  • Duplicates the functionality of Instant, which will lead to confusion about which one to use.

  • Error-prone when representing a LocalDateTime in the future in an implicit time zone, due to possible rules change.

That's not correct. It's pretty reliable at representing LocalDateTime and that's the intended use-case. This is also why it's not duplicating the functionality of Instant and like I said above it's very important to prevent loss of information under serialization. The only valid comparison that is left is ZonedDateTime vs. Pair<LocalDateTime, TimeZone> but to me it's pretty obvious that people will prefer ZonedDateTime.

It's interesting that neither ZonedDateTime nor OffsetDateTime is fully suitable for representing a future event with the ground truth being a LocalDateTime in a specific time zone. Looks like some form of a pair of LocalDateTime and TimeZone may be even more useful than a proper ZonedDateTime.

Oh the irony.😄 Sounds like we agree.

@dkhalanskyjb
Copy link
Collaborator

Do you have an example? My impression is that it adapts the Instant to ensure that the LocalDateTime+TimeZone is stable.

Yep, sure.

ZonedDateTime.of(LocalDateTime.parse("2021-03-27T02:16:20"), ZoneId.of("Europe/Berlin")).plusDays(1).plusDays(1).toString() ==
"2021-03-28T03:16:20+02:00[Europe/Berlin]"

Because an intermediate computation (plusDays(1)) fell into the gap, all future results get shifted by an hour in the LocalDateTime.

it's actually LocalDateTime+Offset or LocalDateTime+ZoneId depending on what input you provide

Not the case. I've shown before that, on deserialization, the LocalDateTime part gets changed to preserve the Instant, see above. Now I've shown also that the gap gets corrected.

The same argument can be made for date + time.

It can't because 1) waiting for an hour at 23:31 will start the next day 2) the set of valid times depends on the date.

should stay unmodified in LocalDateTime and auto-correct the underlying Instant because people in that country keep going to work at the same LocalDateTime and even airports might adapt their active hours (some disallow flights after midnight for example). That's why Java's ZonedDateTime is not Instant+TimeZone, but LocalDateTime+TimeZone.

Your argument: "A is good, and ZonedDateTime is good, therefore ZonedDateTime is A", whereas my argument is "ZonedDateTime is not A, A is good, therefore ZonedDateTime is not good". It's easy to observe that ZonedDateTime is not A.

It's pretty reliable at representing LocalDateTime and that's the intended use-case.

Not true.

@cromefire
Copy link
Author

Use #163 for ZonedDateTime specific discussion.

@Kotlin Kotlin deleted a comment from wkornewald Dec 15, 2021
@justingrant
Copy link

justingrant commented Jul 13, 2022

FWIW, in the design of the new Temporal date/time API in ECMAScript (aka JavaScript), we opted not to offer an OffsetDateTime type. (Although we do have a ZonedDateTime type, as discussed in #163 (comment).)

The reason we didn't offer OffsetDateTime was that this type made it easy for developers to perform anti-patterns like performing non-DST-safe arithmetic, measuring the length of a "day" that would be inaccurate near DST transitions, or constructing invalid local date/time values like times skipped by Spring DST transitions. We also wanted to discourage developers from non-DST regions from assuming that their country's UTC offset would never change, because politics is unpredictable! Finally, we didn't want to increase the complexity of an already-large family of types. Time zones are complicated enough without giving developers two different timezone-enabled types to decide between!

There are valid use cases for OffsetDateTime (like having an ergonomic way to parse and model an Instant string while retaining its original values for date, time, and offset) but the workarounds for these use cases were easy enough, and the use cases unusual enough, that we weren't too concerned about a few extra lines of code being required to accomplish them.

In Temporal, we allow developers to create ZonedDateTime instances using a numeric offset like +05:00 as the time zone. We do not, however, allow developers to parse Instant strings into ZonedDateTime. If someone wants a ZonedDateTime with a numeric offset, then they have to parse the string into an Instant and then convert it to a ZonedDateTime using the offset time zone. This extra step prevents accidentally creating ZonedDateTime instances that can't perform DST-safe math.

Note that I'm not suggesting that Kotlin needs a ZonedDateTime type. Your current solution, now that I understand it seems OK. But if you do add ZonedDateTime then I'd suggest some skepticism about whether OffsetDateTime is actually needed.

dkhalanskyjb added a commit that referenced this issue Feb 20, 2024
Fixes #39
Fixes #58
Fixes #90
Fixes #128
Fixes #133
Fixes #139
Fixes #211
Fixes #240
Fixes #83
Fixes #276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatters Related to parsing and formatting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

12 participants