Yuri6037 / time-tz

Implementation of tz database (IANA) for time Rust crate.
BSD 3-Clause "New" or "Revised" License
22 stars 7 forks source link

Finding start of day in specific time zone #18

Open LinusU opened 10 months ago

LinusU commented 10 months ago

Hello,

I'm currently using this library to bin events into days in a specific time zone.

I have a list of events which occurred at a specific point in time. These event have no concept of time zone. We could represent them with ISO 8601 timestamps in coordinated universal time.

Now I want to know which Swedish day this event belongs to. In order to to this I first convert this timestamp to a zoned date time in the Stockholm time zone.

fn start_of_swedish_day(instant: OffsetDateTime) -> OffsetDateTime {
    let zoned_date_time = instant.to_timezone(timezones::db::europe::STOCKHOLM);

Now that I have a local date, I set the time to 00:00, representing the Swedish midnight.

    let zoned_midnight = zoned_date_time.replace_time(time::Time::MIDNIGHT);

Finally, I want to turn this back into a UTC timestamp.

    zoned_midnight.to_timezone(timezones::db::UTC)
}

This works great for the most time, but breaks when the date is transition over a daylight savings change.


Example:

Let's start with the timestamp 2023-03-26T06:00:00.000Z, in Swedish time this is 8:00 AM.

Note that even though the wall clock says 8:00 AM, we are actually only 7 hours into this day. This is because a minute after 1:59 AM is 3:00 AM on the Sunday of March 26 2023.

After running it thru the start_of_swedish_day we get back the timestamp 2023-03-25T22:00:00.000Z. This is incorrect as at this timestamp the wall clock in Sweden would say 23:00 PM on the 25th.


I'm not sure if I have misunderstood how to use the APIs, or if this is a bug the implementation of replace_time, or something else. Any help would be appreciated!

LinusU commented 10 months ago

Here is a failing test case:

#[test]
fn handles_replace_time_in_timezone() {
    assert_eq!(
        datetime!(2023-03-26 6:00 UTC)
            .to_timezone(timezones::db::europe::STOCKHOLM)
            .replace_time(time::Time::MIDNIGHT)
            .to_timezone(timezones::db::UTC),
        datetime!(2023-03-25 23:00 UTC)
    );
}
LinusU commented 10 months ago

Managed to find a workaround, but I'm still not sure I understand if this should be needed or not 🤔

fn start_of_swedish_day(instant: OffsetDateTime) -> OffsetDateTime {
    let zoned_date_time = instant.to_timezone(timezones::db::europe::STOCKHOLM);
    let zoned_midnight = zoned_date_time.replace_time(time::Time::MIDNIGHT);

    // Re-interpret zoned_midnight as being in the Stockholm time zone
    let primitive_midnight = PrimitiveDateTime::new(zoned_midnight.date(), zoned_midnight.time());
    let zoned_midnight = primitive_midnight.assume_timezone(timezones::db::europe::STOCKHOLM).unwrap_first();

    zoned_midnight.to_timezone(timezones::db::UTC)
}    
Yuri6037 commented 10 months ago

The reason why it does not work is because to_timezone always assumes that the OffsetDateTime you passed is UTC so it doesn't care of the DST times.

When you call replace_time, you replace the time part without leaving any chance to the crate to re-calculate the timezone. In other words, once you call to_timezone you shouldn't modify the OffsetDateTime or you need to recalculate the timezone which is currently only doable through the example code you've shown using PrimitiveDateTime and assume_timezone.

I know this API may look a bit unsound but I couldn't find a better solution. That said, I can try to add a check in to_timezone to handle the case where the OffsetDateTime is not UTC and run through the proper get_offset_local function, however that would be a breaking change as currently to_timezone does not return OffsetResult.

EDIT: You can also do this if all you want is midnight represented in the timezone of your choice: datetime!(2023-03-26 0:00).assume_timezone(timezones::db::europe::STOCKHOLM).unwrap_first()

LinusU commented 10 months ago

Thank you for looking at my issue!

Coming back to this today, I realise that this might actually be expected behaviour since it's called OffsetDateTime, not ZonedDateTime. I expected my OffsetDateTime to keep track of Europe/Stockholm, but what it's actually keeping track of is the offset +02:00, which makes sense giving the name.

Here is how ZonedDateTime from the JS temporal specification behaves:

Screenshot 2023-09-21 at 15 04 51

Do you think that I have understood this correctly?


So maybe this is a feature request for a ZonedDateTime, although that might be out of scope for this library?

Yuri6037 commented 10 months ago

Thank you for looking at my issue!

Coming back to this today, I realise that this might actually be expected behaviour since it's called OffsetDateTime, not ZonedDateTime. I expected my OffsetDateTime to keep track of Europe/Stockholm, but what it's actually keeping track of is the offset +02:00, which makes sense giving the name.

Here is how ZonedDateTime from the JS temporal specification behaves:

Screenshot 2023-09-21 at 15 04 51

Do you think that I have understood this correctly?

So maybe this is a feature request for a ZonedDateTime, although that might be out of scope for this library?

I like the idea of a ZonedDateTime, I don't think such a feature would be out of scope but first I'd have to write down what the API might look like.

Yuri6037 commented 10 months ago

I've just made a first version of ZonedDateTime. I've adapted and made your test-case work:

    #[test]
    fn handles_replace_time_in_timezone() {
        assert_eq!(
            datetime!(2023-03-26 6:00 UTC)
                .to_zoned_date_time(timezones::db::europe::STOCKHOLM)
                .replace_time(time::Time::MIDNIGHT)
                .to_offset_date_time().unwrap_first(),
            datetime!(2023-03-25 23:00 UTC)
        );
    }

Can you try the latest changes on master? Use a git based dependency for now as I don't currently have access to the machine with the crates.io publish key.

LinusU commented 10 months ago

I started testing this out and realised that I needed some more functions from OffsetDateTime, so copied in the functions and started modifying them. That's when I realised that there are some questions on how this should work.


First up was Add<Duration>. I intended to follow RFC 5545 (iCalendar), which is also what the Temporal ZonedDateTime does.

Good explanation of why These rules make arithmetic with `Temporal.ZonedDateTime` "DST-safe", which means that the results most closely match the expectations of both real-world users and implementers of other standards-compliant calendar applications. These expectations include: - Adding or subtracting days should keep clock time consistent across DST transitions. For example, if you have an appointment on Saturday at 1:00PM and you ask to reschedule it 1 day later, you would expect the reschedule appointment to still be at 1:00PM, even if there was a DST transition overnight. - Adding or subtracting the time portion of a duration should ignore DST transitions. For example, a friend you've asked to meet in in 2 hours will be annoyed if you show up 1 hour or 3 hours later. - There should be a consistent and relatively-unsurprising order of operations. If results are at or near a DST transition, ambiguities should be handled automatically (no crashing) and deterministically.

Unfortunately, that doesn't work with the Duration from time, since it normalizes all durations at creation.

E.g. one would think that zoneddatetime!(2023-01-01 22:00 Stockholm) + Duration::days(1) would equal zoneddatetime!(2023-01-02 22:00 Stockholm).

and zoneddatetime!(2023-03-25 22:00 Stockholm) + Duration::days(1) would equal zoneddatetime!(2023-03-26 22:00 Stockholm) since that is "1 day later"

but zoneddatetime!(2023-03-25 22:00 Stockholm) + Duration::hours(24) would equal zoneddatetime!(2023-03-26 23:00 Stockholm)) since that is "24 hours later".

Similarly zoneddatetime!(2023-03-26 01:00 Stockholm) + Duration::hours(1) should equal zoneddatetime!(2023-03-26 03:00 Stockholm) since that is "1 hour later".

This could be fixed by having another Duration struct...


Then there is the question about what to do with invalid dates, this applies both to new, but also replace_time and friends.

e.g. what should ZonedDateTime::new(datetime!(2023-03-26 02:30), timezones::db::europe::STOCKHOLM) return?

and what should zoneddatetime!(2023-03-26 01:00 Stockholm).set_time(time!(02:30)) return?


Finally, why did you make date() and time() return OffsetResult<_>?

Yuri6037 commented 10 months ago

I started testing this out and realised that I needed some more functions from OffsetDateTime, so copied in the functions and started modifying them. That's when I realised that there are some questions on how this should work.

First up was Add<Duration>. I intended to follow RFC 5545 (iCalendar), which is also what the Temporal ZonedDateTime does.

Good explanation of why These rules make arithmetic with Temporal.ZonedDateTime "DST-safe", which means that the results most closely match the expectations of both real-world users and implementers of other standards-compliant calendar applications. These expectations include:

  • Adding or subtracting days should keep clock time consistent across DST transitions. For example, if you have an appointment on Saturday at 1:00PM and you ask to reschedule it 1 day later, you would expect the reschedule appointment to still be at 1:00PM, even if there was a DST transition overnight.
  • Adding or subtracting the time portion of a duration should ignore DST transitions. For example, a friend you've asked to meet in in 2 hours will be annoyed if you show up 1 hour or 3 hours later.
  • There should be a consistent and relatively-unsurprising order of operations. If results are at or near a DST transition, ambiguities should be handled automatically (no crashing) and deterministically.

Unfortunately, that doesn't work with the Duration from time, since it normalizes all durations at creation.

E.g. one would think that zoneddatetime!(2023-01-01 22:00 Stockholm) + Duration::days(1) would equal zoneddatetime!(2023-01-02 22:00 Stockholm).

and zoneddatetime!(2023-03-25 22:00 Stockholm) + Duration::days(1) would equal zoneddatetime!(2023-03-26 22:00 Stockholm) since that is "1 day later"

but zoneddatetime!(2023-03-25 22:00 Stockholm) + Duration::hours(24) would equal zoneddatetime!(2023-03-26 23:00 Stockholm)) since that is "24 hours later".

Similarly zoneddatetime!(2023-03-26 01:00 Stockholm) + Duration::hours(1) should equal zoneddatetime!(2023-03-26 03:00 Stockholm) since that is "1 hour later".

This could be fixed by having another Duration struct...

One could also re-assemble the Duration type through calculations. EDIT: I think I get what you're after: you want to differentiate the user adding days to the user adding hours even if the user adds 24 hours which is also 1 day and treat differently added hours to added days. Not sure if I'd call such a type Duration.

Then there is the question about what to do with invalid dates, this applies both to new, but also replace_time and friends.

What do you mean by invalid date? You don't call new manually, you call to_zoned_date_time on an OffsetDateTime or a PrimitiveDateTime. EDIT: About invalid dates (at least if by invalid you mean non-existent in the specified timezone), they are detected once you call to_offset_date_time which returns an OffsetResult. Would you rather prefer it to be checked before?

e.g. what should ZonedDateTime::new(datetime!(2023-03-26 02:30), timezones::db::europe::STOCKHOLM) return?

Again you're not supposed to call new manually.

and what should zoneddatetime!(2023-03-26 01:00 Stockholm).set_time(time!(02:30)) return?

I don't have any such macro, at first glance I'd say if such a macro needs to be implemented, it would simply create a PrimitiveDateTime assuming the date/time you pass is already in the target timezone.

set_time simply replaces the time part of the PrimitiveDateTime component.

Finally, why did you make date() and time() return OffsetResult<_>?

Because date and time returns the actual date and time in the target timezone, and because the PrimitiveDateTime is assumed to be local, the offset must be re-calculated assuming local date/time which means the function could fail.

Perhaps you considered that date and time shouldn't care about the timezone/DST, in which case the problem is different...

Yuri6037 commented 10 months ago

Update: I've done a version 2 of ZonedDateTime still on master, can you try updating your code with it?

BTW: If you need to add more methods to the new ZonedDateTime could you please submit the changes as a PR?