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

Can't represent Time with specific offset #2490

Closed
ysbaddaden opened this issue Apr 21, 2016 · 4 comments · Fixed by #5324
Closed

Can't represent Time with specific offset #2490

ysbaddaden opened this issue Apr 21, 2016 · 4 comments · Fixed by #5324

Comments

@ysbaddaden
Copy link
Contributor

This generates the following issues:

  • Formatting a Time uses the offset for now which has a high chance to be wrong: DST may be applied or not, timezone offset may have change, or ENV["TZ"] was changed; formatted time that isn't UTC is thus likely to be wrong by 30 minutes, 1 hour, or more;
  • Parsing a datetime with a specific offset currently applies the offset by manipulating the time, and loses the offset information.

Follow up to #2481 (comment)

@ysbaddaden
Copy link
Contributor Author

I tried to see if we could cram in the offset into the UInt64.

Offsets are comprised between -12:00 and +14:00, in increments of 15 minutes, thus need a sign (+/-), 0..14 hours and 0, 15, 30 or 45 minutes (bold assumption that it won't change). It could be encoded as a 7 bits integer (sign = 1 bit, hours = 4 bits, minutes = 2 bits).

But we don't have that much available in the UInt64, even if we drop the kind, and reduce the precision to microseconds (instead of a 10th of microsecond), the maximum date (9999-12-31T23:59:59.999) still uses up to 59 bits, so only 5 bits are available.

BTW: even thought Time has microsecond precision, there is no Time#microsecond accessor.

@straight-shoota
Copy link
Member

straight-shoota commented Nov 23, 2017

In #4556 we've agreed that storing only an offset does not help at all, because from that you can't infer what timezone transition rules to apply. A time with offset is merely presentational. To properly handle DST and other quirks, each time instance needs reference to a specific time zone (i.e location where this time applies). From this location's transition rules, the applicable offset at each point in time can be derived.

Time will need a pointer to a location, meaning each instance will require 24 instead of the current 16 bytes (on 64bit).

However, I think we should also have a floating time type without any zone or offset. This would be the current Time struct without kind. This could be named NaiveTime (similar to Rust::Chrono) and embedded into the zoned Time type. I don't think it makes sense to do it the other way round (having naive Time and ZonedTime) because the default Time should be a zoned time, that's what people most commonly need.

While a floating, zoneless time could theoretically be indicated by a special location, it makes more sense to have a dedicated type for this: For one, it gives type safety. Floating times should not be confused with absolute time instances. And it also saves space, so if you have an application where you need to store a lot of time instances wich don't need a location, because they all share the same or is not important, you can just use a naive time.

I've been experimenting with a Location type with a reader for binary time zone data, inspired by Go's implementation which seems very reasonable for this. This basic step was fairly easy but the more complex task will be proper integration into Crystal (though I have already replaced Time::Kind successfully with minimal effort and most specs passing) and os environment.

I'm still refactoring a bit, but I will upload a working branch later.

@ysbaddaden
Copy link
Contributor Author

Im afraid we wont accept having different implementations for time. There should be just one Time struct that works with offsets, not different solutions with different features that are confusing, or require lots ofto_time calls.

@straight-shoota
Copy link
Member

Adding this naive time implementation would just be an additional step, anyway. So we don't need to discuss this in detail for now now and we should probably keep this other detail for a subsequent issue. The first thing is supporting proper timezones for Time.
I just thought I'd mention this as the (for me) logical next step.

This was referenced Nov 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants