-
Notifications
You must be signed in to change notification settings - Fork 262
Reduce year validity range to 500,000 #7533
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
base: main
Are you sure you want to change the base?
Conversation
82f7a38 to
f5751a3
Compare
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discovered that 7-digit years do not work in IXDTF.
How does that imply this? We don't have code to infallibly produce IXDTF strings from Dates afaik.
Does the IXDTF spec actually define a limit? "do not work" and "aren't allowed" are separate things.
|
So, looking deeper into this, it's interesting. Temporal specifies it as 4 digits, or 6 digits with a sign. This is what we implement. This matches older JS-spec requirements. Temporal describes its spec as basically being an ISO 8601/RFC 9557 hybrid. There's some discussion about this here. ISO 8601 has a concept of "expanded representations"
Basically, you can use + or - before a year to say it is "expanded", and then "expanded" years MUST have a fixed number of digits (if it's 6, you specify a 5-digit year with a leading IXDTF (RFC 9557, building on RFC 3339) itself doesn't specify anything about expanded years, though it does basically say it's building on ISO 8601 as a profile for it. So, this is a Temporal spec thing, which is conformant to but not specified by ISO 8601. Our ixdtf parser implements the Temporal variant. |
fd4a515 to
ee1a6a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is a Temporal spec thing, which is conformant to but not specified by ISO 8601. Our ixdtf parser implements the Temporal variant.
Ok. But this still doesn't require our non-IXDTF Date constructors to be limited to 500'000 years.
| /// - Temporal's IXDTF only supports up to 6-digit years (ISO 8601 requires expanded years to | ||
| /// specify a digit count, Temporal [specifies](https://tc39.es/proposal-temporal/#sec-temporal-iso8601grammar) | ||
| /// it as six digits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is relevant to our internal range. The string range can be smaller than what we can represent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is listed under "some things to consider". It is relevant, and should absolutely be considered. We are actively considering it now.
If someone wishes to change these limits again they can consider it again. I don't see why this should not be left around as a breadcrumbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole PR seems to be motivated by us having IXDTF serialization. We don't have IXDTF serialization, and there's no open issue for adding it. Is this something we're actually planning?
The IXDTF string is the reliable serialization format for all of our dates. I think it's an important property that we can actually serialize all of our dates. The 100000 limit was arbitrarily chosen as "larger than Temporal". This is also arbitrarily chosen but it maintains more properties. That seems good to me. If we support a date we should support as many operations as possible. If I had my druthers we would actually have extremely narrow date ranges for most calendars. We should not have had to spend so much time in ICU4X and Temporal debating what the behavior of the Chinese calendar outside of our data range should be: the calendar should just end there. But choices made in Temporal forced our hand here. So while we support them, I think these dates are basically pointless and "reducing the range" has no downside (given that these are pointless) provided we don't reduce past the Temporal range. |
ee1a6a6 to
805dc28
Compare
805dc28 to
b7822b6
Compare
|
We should always roundtrip through the string constructors because they are for serialization. If the string constructors have an upper year limit, then we should keep all Dates within that upper limit. |
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If IXDTF supports 6-digit years, we should as well. A limit of 500000 only supports half of them, so we should set the limit at 999999
This is a departure from the previous strategy of setting the limit based on calendar years. We can't set a limit of 999999 because it is possible to create dates in calendars like ROC and Julian that map to ISO years greater than that. It is exactly what this PR is trying to fix. |
|
Alternatively, although it would be a departure from the current strategy, I would be okay setting the calendar year limits at +/- 500,000 and the RD limits at -999999-01-01 to +999999-12-31. |
|
I think we need to have a holistic discussion around the ranges, including arithmetic. I don't think this PR should be merged in any shape until we have figured that out.
Again, this is only required for IXDTF serialization. We don't have IXDTF serialization. |
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
Yeah, not a fan, because I think that the model we have settled on with soft year ranges and a hard RD range is a really nice one.
Temporal's IXDTF variant supports 6 digit years, and it only supports them to ±270000. IXDTF and ISO in general, as mentioned, don't specifically provide a number for this. I think "ICU4X can deserialize dates it serializes" plus "ICU4X can deserialize and support all Temporal dates" are sufficient properties here. I do not think "ICU4X can deserialize all dates supported by the grammar it parses" is a useful property since nobody is producing those. I think this is the best route especially when you factor in Temporal client needs. |
Hm? This retains soft year range and hard RD range, just changes the way the RD range is derived. |
|
Hmmm, I guess so? I kind of like that the hard range is "tight". |
|
I am warming up to the idea of a wider RD range. I think if we set the RD range to all parseable Temporal dates, and then update #7545 to check the RD range. A thing that helped change my mind is that my current plan requires #7531 to introduce a "field: RataDie" for the range error, which is kind of ugly and confusing. Whereas if our RD range is all dates parseable by the I still like Robert's Footnotes
|
We discovered that 7-digit years do not work in IXDTF. Reducing to 500k, instead, which has a decent buffer between the Temporal lower bound and the IXDTF upper bound.
900k would also probably be acceptable if we want to reduce slowly.