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 struct initialization with dates greater than December, 31st 9999 #12559

Open
Pluvie opened this issue Oct 4, 2022 · 11 comments
Open

Time struct initialization with dates greater than December, 31st 9999 #12559

Pluvie opened this issue Oct 4, 2022 · 11 comments

Comments

@Pluvie
Copy link

Pluvie commented Oct 4, 2022

Hi all!
First of all thank you and congratulations for developing such an amazing language!
I've been a hardcore loving Rubyist for 10+ years now, but since I've discovered Crystal some months ago, I've been seriously in doubt about what of the two would be my favorite language 😄 and that definitely didn't happen with all the Rust, Go, Elixir, Clojure etc. experiences!

Anyhow, I just wanted to discuss a little improvement on the Time struct to allow the creation of dates greater than December, 31st 9999. I've looked into the source code, at crystal/src/time.cr:476 and I've seen that if the internal @seconds of the Time are greater than Time::MAX_SECONDS then an ArgumentError.new "Invalid time: seconds out of range" is raised.

Since I'm still a newbie of the language, I might miss something out, but I'm asking myself the reason why there is this limit, and didn't find an answer so far. What I thought is that, technically, since @seconds is a Int64 the check should be made on Int64::MAX, not the arbitrary Time::MAX_SECONDS. But of course you cannot even create an Int64 greater than the max (Arithmetic overflow error), so I think that just removing that check is sufficient.

One particular reason about this change is this: I'm developing a BSON specification implementation in pure Crystal (no C bindings). One of the BSON corpus tests is failing because I cannot instantiate dates greater than 31/12/9999 (see https://github.com/mongodb/specifications/blob/master/source/bson-corpus/tests/datetime.json line 25, where the test asks you to create a BSON object with a field with date 01/01/10000).

What do you think about this? Thanks!

@straight-shoota
Copy link
Member

Thanks for raising this issue.

The restriction on maximum date at the end of year 9999 has been in place since the Time struct was introduced in Crystal. It's based on Mono's System.DateTime which has the same restriction.
The calendar definition is based on ISO 8601, which defines a year as generally represented by a four digit number in the range between 0000 and 9999. It allows for an extended representation for dates outside that range, though.
But 0..9999 is a commonly used and pretty practical standard range. I mean there are little practical use for dates that far away from the current. And its easy to assume years have a maximum of 4 digits for formatting and parsing algorithms.

That said, I don't think there's a strict requirement to keep this limitation in place.

But removing that single check is not sufficient. The limitation on the max value of representable dates is encoded everywhere in the Time API, from interfaces to algorithms. It would require a careful approach to identify what this affects and how to resolve it.
This shouldn't be terribly complex and can easily explored iteratively by adding specs using expanded representations for all time methods. It's still quite some effort. And the benefits aside from helping to fulfill a BSON spec are not entirely clear to me.
So I wouldn't give this high priority.

@asterite
Copy link
Member

asterite commented Oct 4, 2022

You could also check whether there's a BSON library for C#, and whether it conforms to the spec, and how, given that C# has the same limitation... and then take the same approach, which is probably one of these two:

  1. Ignore the failing spec, as in practice it's rare that someone will use a date that far in the future
  2. Use their workaround

@Pluvie
Copy link
Author

Pluvie commented Oct 4, 2022

Dear @asterite, thanks for the suggestion and sorry I forgot to mention that I've already researched into that.
There is infact another BSON implementation in pure Crystal (https://github.com/elbywan/bson.cr) that handles the problem using your first proposed solution. Citing from https://github.com/elbywan/bson.cr/blob/master/spec/corpus/README.md:

Y10K datetime raises an "Invalid time: seconds out of range" error.
Added the "ignore_json_roundtrip" flag to ignore round trip tests.

Which is the exact same thing that I'm doing right now, in order to get the spec to pass.
I was just proposing the change in Crystal merely for freedom, as I thought that, if no technical limit or reason exists, I should be able to create a date even in the distant future.

Thinking about another application where it could be potentially useful: maybe in astronomical computations where planet conjunctions, comet pass by's or things like that are calculated in the next thousands of years?

Anyway, thanks again for your feedback! Let's say that you all agree with the proposed change.
Do you honestly think that I could be of any help by starting a change like @straight-shoota proposed (with careful approach and added specs) or is it something too high for a newbie?

@asterite
Copy link
Member

asterite commented Oct 4, 2022

I'm fine with extending the range of Time to support years greater than 9999. I also think this is a good task for you to take, @Pluvie , if you want.

But removing that single check is not sufficient. The limitation on the max value of representable dates is encoded everywhere in the Time API, from interfaces to algorithms. It would require a careful approach to identify what this affects and how to resolve it.
This shouldn't be terribly complex and can easily explored iteratively by adding specs using expanded representations for all time methods.

Right, I think going over the Time specs and making sure values greater than 9999 are supported should be pretty straight-forward.

@j8r
Copy link
Contributor

j8r commented Oct 4, 2022

It is also not currently allowed to create a 0 year date: Time.utc(year: 0, month: 1, day: 1) will raise an exception.
Maybe negative years could also be supported additionally?

@straight-shoota
Copy link
Member

Thinking about another application where it could be potentially useful: maybe in astronomical computations where planet conjunctions, comet pass by's or things like that are calculated in the next thousands of years?

I don't think a general purpose calendar API is really suitable for astronomical computations. So I wouldn't see that as a practical use case.
Another common example are historical or archeological applications for years before the epoch. But this isn't really applicable either. It's possible to place an exact date on certain historical events. For example, Caesar crossing the Rubicon on January 10, 49 BC. But relating that to a specific instant on the time line is basically impossible. This date representation is not even in the ISO-8601 calendar, which really doesn't work before the year 1582. And if it was, there's too much uncertainty with ancient dates that it doesn't make much sense to use an instant-based representation of time and date. For such special purposes you need a special purpose calendar and date model.

I think it still wouldn't hurt to provide a wider span of values. FWIW Ruby supports years in truely astronomical ranges (the API notes.

@Pluvie
Copy link
Author

Pluvie commented Oct 5, 2022

Thanks again for the feedback guys!
So if you all agree, I will happily try my best to work on the implementation as you suggested. Let me know if that's OK.

My approach would be, at a first stage, to only focus only on extending the maximum date with @seconds up until Int64::MAX, which means the date 3384805-12-07 07:48:20 +0000: make sure that it does not break anything, and provide new specs to describe the extended range.

I've already read the contributing guidelines, if there is something else to know please do tell me.
This would be my first time contributing to a big open source project!

@yxhuvud
Copy link
Contributor

yxhuvud commented Oct 5, 2022

It is also not currently allowed to create a 0 year date

That is a good thing because AFAIK year 0 never was. It went from 1BC to 1AD.

@straight-shoota
Copy link
Member

It is also not currently allowed to create a 0 year date

That is a good thing because AFAIK year 0 never was. It went from 1BC to 1AD.

Yes and no. Depends on the calendar, or more specifically the year numbering. The ISO-8601 calendar uses astronomical numbering, which includes a year zero. It's equivalent to 1BC. That's for continuity reasons. Having a gap between year 1BC and 1AD would be catastrophic for calendrical calculations.

@straight-shoota
Copy link
Member

My approach would be, at a first stage, to only focus only on extending the maximum date with @seconds up until Int64::MAX, which means the date 3384805-12-07 07:48:20 +0000: make sure that it does not break anything, and provide new specs to describe the extended range.

I don't think your math is right. The biggest datetime representable with 64-bits and calendar epoch 0001-01-0 should be 292277024627-12-06 14:36:39 (according to Ruby: (Time.new(1, 1, 1) + 9223372036854775807).utc).
The same proves the following Crystal program:

SECONDS_PER_DAY = 86400
DAYS_PER_YEAR = 365.2425
EPOCH_YEAR = 1
p! (Int64::MAX / SECONDS_PER_DAY / DAYS_PER_YEAR + EPOCH_YEAR) # => 292277024627.9277

Note that we can't go up entirely to Int64::MAX though. We need to at least account for a UTC offset on top.

@HertzDevil
Copy link
Contributor

HertzDevil commented Oct 5, 2022

If Time#year continues to return an Int32 the actual usable range is much smaller than that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants