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

Calculate TimeZoneInput from timestamps #533

Closed
sffc opened this issue Mar 5, 2021 · 10 comments
Closed

Calculate TimeZoneInput from timestamps #533

sffc opened this issue Mar 5, 2021 · 10 comments
Labels
C-time-zone Component: Time Zones S-large Size: A few weeks (larger feature, major refactoring) T-core Type: Required functionality U-fuchsia User: Fuchsia

Comments

@sffc
Copy link
Member

sffc commented Mar 5, 2021

Pending #528, we will likely require time zones to be fully resolved into GMT offsets and metazones. However, the reality is that people will want to hand ICU4X a timestamp, and get a localized result out the other end. Our organization has the expertise to implement these conversions, and I think we should release libraries to perform them. Of course, they should be modular and stand to the side of ICU4X.

What we should implement is, I think, rather straightforward:

* I would like to take the BCP-47 time zone identifier as input because I want to encourage the use of the small, fixed-width, lightweight types within ICU4X. However, since most people will be using IANA identifiers, we should also provide a library on the side that can convert from IANA identifier to BCP-47 identifier.

We should also think about how this relates to the ISO-8601 and Temporal data models. In particular, we may want to match our data model to strings such as:

  1. 2021-03-05T07:49:02Z is a timestamp in UTC
  2. 2021-03-05T00:49:02-0700 is the same timestamp but with a GMT offset
  3. 2021-03-05T00:49:02-0700[America/Denver] is a zoned datetime; the GMT offset should serve as a hint to help resolve potential conflicts near variant transitions (daylight savings time)

In the end, we also need the ISO date to feed into #534, so the time zone resolution layer should make sure to retain that information when possible.

@sffc sffc added T-core Type: Required functionality discuss Discuss at a future ICU4X-SC meeting C-datetime Component: datetime, calendars, time zones labels Mar 5, 2021
@sffc sffc changed the title Implement timezone conversion Implement timestamp-to-timezone resolution Mar 5, 2021
@sffc
Copy link
Member Author

sffc commented Mar 12, 2021

Discussion 2021-03-12:

  • This component will need DataProvider and is unlikely to be useful outside the context of ICU4X
  • We will release this as an ICU4X utility, living in the ICU4X repo
  • This component is more closely tied to ICU4X than, say, TinyStr, so we shouldn't tie this conclusion to conclusions about TinyStr

@sffc sffc removed the discuss Discuss at a future ICU4X-SC meeting label Mar 12, 2021
@sffc sffc added this to the 2021-Q2-m1 milestone Mar 12, 2021
@sffc sffc changed the title Implement timestamp-to-timezone resolution Implement time zone calculations Mar 16, 2021
@sffc
Copy link
Member Author

sffc commented Mar 16, 2021

I think we should seriously consider making a TimeZone class with a similar API as Temporal.TimeZone, which has two benefits:

  1. We can follow a pre-existing design
  2. Helps ICU4X be a basis for Temporal implementations

Read more: https://github.com/tc39/proposal-temporal/blob/main/docs/timezone.md

CC @nordzilla

@nordzilla
Copy link
Member

I will incorporate this into Friday's discussion.

@nordzilla nordzilla added the S-large Size: A few weeks (larger feature, major refactoring) label Apr 3, 2021
@sffc sffc modified the milestones: 2021-Q2-m1, ICU4X 0.4 Apr 19, 2021
@nordzilla nordzilla changed the title Implement time zone calculations Calculate TimeZoneInput from timestamps Jul 22, 2021
@nordzilla nordzilla removed their assignment Aug 25, 2021
@sffc sffc added the help wanted Issue needs an assignee label Aug 26, 2021
@nordzilla nordzilla modified the milestones: ICU4X 0.4, ICU4X 0.5 Aug 26, 2021
@nordzilla nordzilla modified the milestones: ICU4X 0.5, ICU4X 1.0 Jan 20, 2022
@nordzilla nordzilla self-assigned this Jun 6, 2022
@sffc sffc modified the milestones: ICU4X 1.0 (Features), ICU4X 1.1 Jul 28, 2022
@nordzilla nordzilla added the blocked A dependency must be resolved before this is actionable label Aug 16, 2022
@nordzilla nordzilla removed the blocked A dependency must be resolved before this is actionable label Aug 16, 2022
@nordzilla nordzilla modified the milestones: ICU4X 1.1, ICU4X 1.2 Dec 15, 2022
@sffc sffc added C-time-zone Component: Time Zones and removed C-datetime Component: datetime, calendars, time zones labels Dec 22, 2022
@sffc sffc added the U-fuchsia User: Fuchsia label Jan 25, 2023
@sffc
Copy link
Member Author

sffc commented Jan 25, 2023

Note that we will want to support both directions. In ICU these are Calendar::setTime and Calendar::getTime.

@robertbastian
Copy link
Member

Fixed by #5349

@sffc
Copy link
Member Author

sffc commented Sep 17, 2024

#5349 doesn't let us consume a timestamp, which is the title of this issue. We could decide that this issue is out of scope and close it, but that would be a TC decision; the stated issue is definitely not "fixed by" #5349.

@sffc sffc reopened this Sep 17, 2024
@sffc sffc added discuss Discuss at a future ICU4X-SC meeting and removed help wanted Issue needs an assignee labels Sep 17, 2024
@robertbastian
Copy link
Member

robertbastian commented Sep 17, 2024

Oh sorry, I only saw the IXDTF string in the issue and didn't read it in enough detail.

I don't think we want to do this. Going from timestamp + zone identifier to datetime + GMT UTC offset requires the full TZDB, this is what we want to rely on crates like chrono_tz and jiff for. Going from datetime + zone identifier + UTC offset to metazone and zone variant is implemented. I think this is fixed.

@sffc
Copy link
Member Author

sffc commented Sep 17, 2024

Oh sorry, I only saw the IXDTF string in the issue and didn't read it in enough detail.

I don't think we want to do this. Going from timestamp + zone identifier to datetime + GMT UTC offset requires the full TZDB, this is what we want to rely on crates like chrono_tz and jiff for.

It is only in the last 14 days that my thinking has shifted toward us taking a path that avoids the full TZDB in ICU4X. I think we should still have the TC discussion to decide on whether this opinion is shared with others.

@robertbastian robertbastian added the discuss-priority Discuss at the next ICU4X meeting label Jan 20, 2025
@robertbastian
Copy link
Member

  • @sffc Background: @robertbastian and I and Justin have been working on formatting time zones in a way that doesn't use TZDB at runtime. We do this by deriving a data store compiled ahead of time from TZDB to store display names, and then using the zone offset to look up display names. This allows us to store only 10KB of data, which is a large space savings, since the TZDB display names change a lot less frequently. The tradeoff is that ICU4X would not be able to format a timestamp (seconds from the UNIX epoch), and instead, users would need to use an external library to convert epoch seconds in a way that uses TZDB to format that fully.
  • @robertbastian The big problem with doing time zone calculations in ICU4X is that the tzdb updates a lot more often than CLDR does. If we include a copy of the TZDB, a lot more clients would need to ship their own TZDB anyway. We could read the TZDB from the OS, etc., but we're focused on being portable, and there's a lot of work to do there. jiff has already done that work. I don't think we need to repeat that work. And we are somewhat working with jiff to provide interop. @BurntSushi is working on a crate to improve that interop, so you can use jiff to get your zoned date time and then use ICU4X to display it.
  • @hsivonen Is jiff semantically compatible with Temporal? Policies on leap seconds and stuff like that.
  • @sffc jiff takes inspiration from ECMAScript Temporal. But Temporal_rs is the crate that is intended to be Temporal compatible. I think @BurntSushi would agree that jiff is the Rust-forward implementation, and Temporal_rs is meant to be bug-for-bug compatible with Temporal.
  • @hsivonen So to understand the proposal, you're saying that live access to the TZDB is out of scope for ICU4X, and we'd tell applications that want to do time zone calculations to use a combination of jiff and ICU4X?
  • @robertbastian Currently, mostly yes. There are some formats that don't need TZDB. For those that do, you need to give us the subset of time zone information we need for the specific format. You could even do your time zone calculations on a server somewhere, and send it in serialized IXDTF format to ICU4X.
  • @hsivonen For the analysis in teh spreadsheet, some lines are needed for ECMA-402 coverage. For the line items that relate to time zones, if it's out of scope for ICU4X, then we tell users "go use jiff".
  • @sffc I wanted to indicate that the spreadsheet already has line items that are out of scope, and this would be one more such line item out of scope.
  • @zbraniecki I was hoping temporal_rs would be the blessed pair. In terms of coverage, I don't mind extracting auxiliary pieces to ICU4X.
  • @robertbastian About temporal_rs vs jiff in Rust, I think jiff has more potential to become a standard Rust crate.
  • @zbraniecki temporal_rs is more cross-language.
  • @sffc I think part of the idea here is you use whatever datetime library is best. You could use temporal_rs over FFI, or you could use java.time or anything else.
  • @robertbastian - For FFI we might want to expose either temporal_rs or jiff functionality, as C and C++ don't have OOTB time zone calculations
  • @sffc Are you both aligned on ICU4X not using TZDB at runtime?
  • @zbraniecki I'm comfortable with this approach.
  • @hsivonen And we're sure that there is another crate that does this?
  • @robertbastian Yes, there are at least 3 full implementations in Rust.

Consensus: ICU4X will defer to other crates or libraries on calculating time zone offsets and transforming epoch seconds into zoned datetimes.

LGTM: @zbraniecki @sffc @robertbastian @hsivonen @Manishearth

@BurntSushi
Copy link

Is jiff semantically compatible with Temporal? Policies on leap seconds and stuff like that.

For leap seconds, yes: they are ignored (except that something like 23:59:60 is parsed as 23:59:59, which is what Temporal does).

I think it was mentioned in your meeting, but to confirm, yes, Jiff is not intended to match the Temporal spec precisely. When I started building Jiff, I don't think I even knew about Temporal. I just wanted to build a great datetime library. It turned out that the clearest path to that goal was to follow in Temporal's footsteps.

I should probably write a doc that attempts to more exhaustively enumerate the differences. Jiff does support more stuff than Temporal, but just focusing on where things might be different:

  • There are some differences in what kinds of RFC 9557 datetimes are supported.
  • As mentioned in that link, but to call out clearly, Jiff's datetime range is -9999 to 9999. Temporal's range is a fair bit bigger.
  • jiff 0.2 will forbid implicit assumptions that days are 24 hours in Span (Jiff's equivalent to Temporal's Duration) APIs. You can still opt into this without a relative date using a special marker.
  • Jiff doesn't support overflow: constrain. It's just overflow: reject. (No particular reason. I plan to add it some day.)
  • As y'all probably already know, Jiff has no support for non-Gregorian calendars or localization. Hence the desire to promote integration between Jiff and icu. :-)

I think there are probably other differences. But overall my approach has been to strongly defer to Temporal wherever possible.

@sffc sffc removed discuss Discuss at a future ICU4X-SC meeting discuss-priority Discuss at the next ICU4X meeting labels Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-time-zone Component: Time Zones S-large Size: A few weeks (larger feature, major refactoring) T-core Type: Required functionality U-fuchsia User: Fuchsia
Projects
None yet
Development

No branches or pull requests

4 participants