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

time: add Time.ZoneBounds #50062

Closed
mjonss opened this issue Dec 9, 2021 · 20 comments
Closed

time: add Time.ZoneBounds #50062

mjonss opened this issue Dec 9, 2021 · 20 comments

Comments

@mjonss
Copy link

mjonss commented Dec 9, 2021

Export Location.lookup() as Location.Lookup() to help finding DST transitions.

Sometimes one want to find the when a zone transition starts or ends, which is already implemented in the non exported func (l *Location) lookup(sec int64) (name string, offset int, start, end int64, isDST bool)

Example: I need to correct '2021-03-28 02:30:00' in 'Europe/Amsterdam', which is in the non-existing hour during DST transition between CET and CEST, to the first coming valid time.
I.e. there is a requirement to have a function that takes a date and time in a DST Location and if it is incorrect, correct it to the first following correct date and time.

Setting the time and then reading it back may give a new time that is not at the DST transition, like: https://go.dev/play/p/sJdTyNZJPT6

And I don't expect time.Date() to change it current behaviour, but if there was an efficient way to find the DST transition one could easily just check the DST transition and correct it one self.

@mjonss mjonss added the Proposal label Dec 9, 2021
@gopherbot gopherbot added this to the Proposal milestone Dec 9, 2021
@ianlancetaylor ianlancetaylor changed the title proposal: x/time: Export Location.lookup() as Location.Lookup() proposal: time: export Location.lookup() as Location.Lookup() Dec 9, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Dec 9, 2021
@ianlancetaylor
Copy link
Contributor

We don't want to simply export the current Location.lookup method. That method takes and returns a number of seconds in the Unix epoch, but an exported method should use time.Time. For an exported API we should probably return some indication of ambiguity, when a time.Time value exists in two different timezones (e.g., when the hour between 1am and 2am is repeated in typical daylight savings time transitions).

Want to propose a useful API to export? Thanks.

@rsc
Copy link
Contributor

rsc commented Dec 15, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Dec 15, 2021
@rsc
Copy link
Contributor

rsc commented Jan 5, 2022

It sounds like the need here is for a new method on time.Time that tells the time zone transitions to either side of the instant that is described by the time.Time? Do I have that right?

@mjonss
Copy link
Author

mjonss commented Jan 5, 2022

It sounds like the need here is for a new method on time.Time that tells the time zone transitions to either side of the instant that is described by the time.Time? Do I have that right?

Yes, the primary need is to find the time zone transitions, since that information already exists, but is not exposed.

@ianlancetaylor
Copy link
Contributor

Thanks. What about

// ZoneBounds returns the bounds of the time zone in effect at time t.
// The zone begins at start and the next zone begins at end.
// If the zone begins at the beginning of time, start will be returned as a zero Time.
// If the zone goes on forever, end will be returned as a Time in the very distant future.
// The Location of the returned times will be the same as t.
func (t Time) ZoneBounds() (start, end Time)

@martin-sucha
Copy link
Contributor

Yeah, that might work. I like that ZoneBounds is simple. There are a couple limitations of that interface though, see below.

If the zone goes on forever, end will be returned as a Time in the very distant future.

Should we expose that future time as a constant or add a method to check for it (similar to IsZero()) to time.Time? Consider a program that prints the next 10 zone transitions is given a location that goes on forever without transitions, like UTC. That program will need to determine when to stop iterating.

How is the user supposed to iterate backwards in time? Subtract Duration(1) from start and call ZoneBounds() on the result?

So, daylight saving time flag, zone name and offset will be exposed with Time.IsDST and Time.Zone, right? IsDST/Zone do lookups currently, is it okay if we first do lookup in ZoneBounds() and then again for IsDST/Zone calls? While we could theoretically cache lookup results in Time, I would be cautious about making Time bigger.

If we wanted iteration that is O(1) instead of needing to do O(log N) lookup every time, we could cache the zone index, but it would require an additional type. What about something like

type Zone struct {
	// location that the zone is part of.
	location *Location
	// index of the current zone in Location.tx.
	// -1 if the first zone (lookupFirstZone) is used.
	// 0..len(Location.tx)-1 for a zone starting with the given transition index.
	// len(Location.tx) if we used extend string to build this Zone.
	index int
	// start time of the zone.
	// Zero time if the zone starts at the beginning of time.
	// This identifies the zone instance when using extend string.
	start Time
}

// Name of the zone.
func (z Zone) Name() string

// Offset of the zone east of UTC.
func (z Zone) Offset() Duration

// Start is the time when the zone starts.
// If the zone begins at the beginning of time, start will be returned as a zero Time and ok will be false.
func (z Zone) Start() (start Time, ok bool)

// End is the time when next zone starts.
// If the zone goes on forever, end will be returned as a Time in the very distant future and ok will be false.
func (z Zone) End() (end Time, ok bool)

// IsDST reports whether the time in the zone is in Daylight Savings Time.
func (z Zone) IsDST() bool

// LaterZone returns the next zone.
// If there is no later zone, LaterZone returns (z, false).
func (z Zone) LaterZone() (later Zone, ok bool)

// EarlierZone returns the previous zone.
// If there is no previous zone, EarlierZone returns (z, false).
func (z Zone) EarlierZone() (earlier Zone, ok bool)

// LookupZone describes the zone in effect at t.
// The Location of the times returned by the Zone will be the same as t.Location().
func (t Time) LookupZone() Zone

Disadvatages:

  • Larger API surface.

Advantages:

  • Efficient iteration of zones.
  • Easy to add a field to the result in the future if needed.

For a use case like building a VTIMEZONE, we could add methods like:

// IsExtended returns true if the Zone was computed from an extend string.
func (z Zone) IsExtended() bool

// Extend returns the zone extend string.
func (l *Location) Extend() string

However, the name Zone might clash with types proposed in #49951. We could use ZoneIter instead of Zone, but I don't like ZoneIter as much as Zone.

@ianlancetaylor
Copy link
Contributor

Should we expose that future time as a constant or add a method to check for it (similar to IsZero()) to time.Time?

Fair question. There is a tension between returning a zero result for the end of time and returning a time in the distant future. Returning a zero result makes it easy to see that there is no information but harder to write a quick comparison to see whether a time is in range. Defining a new constant or method extends the API only for purposes of this new method, which seems unfortunate.

How is the user supposed to iterate backwards in time?

Is this a common operation that needs to be supported efficiently in the low-level time package?

@martin-sucha
Copy link
Contributor

martin-sucha commented Jan 11, 2022

If we are going to expose zone boundaries, we should probably document what a zone boundary is or isn't.

In valid TZif files, it seems that a transition time is when at least one of the following changes (RFC 8536, section 2):

  • UT offset
  • whether daylight saving time is in effect
  • time zone abbreviation

although there is the following in RFC 8536, section 3.2:

Each value is used as a transition time at which the rules for computing local time may change.

so I'm not sure whether it is guaranteed that something changes.

Also I have no idea how the properties of boundaries look like if we are getting the zone data from another source, for example Windows.

@rsc
Copy link
Contributor

rsc commented Jan 12, 2022

If we wanted to do a scan of all known time zone changes at a given location, then assuming there are N of them, that scan takes O(N log N) with the simple API and O(N) with the complex API. But log N is tiny, since N is two per year. Is it really worth all this new API just to save a small constant factor? It seems hard to believe that it would be.

It seems like in the ZoneBounds API, end needs to be zero if there is no future switch, just so that loops can see a value to terminate with.

@rsc
Copy link
Contributor

rsc commented Jan 12, 2022

@mjonss would the API in #50062 (comment) address your use case?

@mjonss
Copy link
Author

mjonss commented Jan 13, 2022

@mjonss would the API in #50062 (comment) address your use case?

That should be enough and I agree with keeping it simple. My specific use case is just check the bounds once when the parsing/conversion of a time does not yield the same result (i.e. due to DST transition). But I can also see other use cases needing to loop over a few years.

Also changing the very distant future to zero Time would make it more complete and still simple to check. So the ZoneBounds() of an UTC location would just return two zero Time.

If performance would be needed, then the data set is also small enough to run once and cache by own implementation.

@martin-sucha
Copy link
Contributor

Also +1 for end to be zero time if the zone goes on forever.

If we wanted to do a scan of all known time zone changes at a given location, then assuming there are N of them, that scan takes O(N log N) with the simple API and O(N) with the complex API. But log N is tiny, since N is two per year. Is it really worth all this new API just to save a small constant factor? It seems hard to believe that it would be.

That is a good point. I agree that it is not worth adding the complex API.

For the use case of scanning all known time zone changes, there is one piece still missing - a zone with an extend string can repeat transitions forever. Although it seems that it is better to have a method like

// Extend returns the location's extend string and the time when it first takes effect.
func (*Location) Extend() (extend string, start time.Time)

instead of exposing that information as part of the zone lookup. Seems that adding the Extend() method or similar could be handled in a separate proposal.

The only remaining question I have is whether we want to add a note to the documentation warning that (some?) zone properties might not change at zone boundaries. For example:

// Return values of Zone or IsDST might change only at zone boundaries,
// but subsequent zones are not required to differ.

Does it make sense?

@mjonss
Copy link
Author

mjonss commented Jan 17, 2022

Since time zones are not stable over time and changes happens frequently, I don't think it makes much sense of iterating over time zone transitions for more than a "few" years (I don't imagine any time zone 'extension' to be left unchanged for more than some 100 years? Hopefully European Union will get rid of theirs eventually...)

@rsc
Copy link
Contributor

rsc commented Jan 19, 2022

The Extend string seems like too much. Those are a cryptic, not terribly well-defined format, and we don't want to expose that as our API for all time.

But adding ZoneBounds seems OK.

@rsc
Copy link
Contributor

rsc commented Jan 19, 2022

It sounds like the current ZoneBounds proposal is:

// ZoneBounds returns the bounds of the time zone in effect at time t.
// The zone begins at start and the next zone begins at end.
// If the zone begins at the beginning of time, start will be returned as a zero Time.
// If the zone goes on forever, end will be returned as a zero Time.
// The Location of the returned times will be the same as t.
func (t Time) ZoneBounds() (start, end Time)

(The difference from iant's comment above is that end is a zero time when there isn't one.)

Do I have that right? Does anyone object to that?

@mjonss
Copy link
Author

mjonss commented Jan 19, 2022

Looks good to me, no objections.

@martin-sucha
Copy link
Contributor

Looks good to me.

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jan 27, 2022
@rsc
Copy link
Contributor

rsc commented Jan 27, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc changed the title proposal: time: export Location.lookup() as Location.Lookup() proposal: time: add Time.ZoneBounds Feb 2, 2022
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Feb 2, 2022
@rsc
Copy link
Contributor

rsc commented Feb 2, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: time: add Time.ZoneBounds time: add Time.ZoneBounds Feb 2, 2022
@rsc rsc modified the milestones: Proposal, Backlog Feb 2, 2022
@gopherbot
Copy link

Change https://go.dev/cl/405374 mentions this issue: time: add Time.ZoneBounds

@dmitshur dmitshur modified the milestones: Backlog, Go1.19 May 17, 2022
@golang golang locked and limited conversation to collaborators May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

6 participants