Update `ixdtf` to handle unbound fraction length #6036

nekevss · 2025-01-25T06:15:34Z

This PR updates ixdtf's fraction handling to parse fraction values beyond 9 digits in length per feedback from #6004.

It introduces a new enum Fraction in place of the previous nanosecond field that allows parsing fraction values beyond 9 digits while preserving the precision in the enum.

It also adds a variety of tests to test the new behavior while adjusting tests from the old behavior to the new behavior

sffc · 2025-01-27T04:37:29Z

utils/ixdtf/src/parsers/records.rs

+pub enum Fraction {
+    /// Parsed nanoseconds value (A fraction value from 1-9 digits length)
+    Nanoseconds(u64),
+    /// Parsed picoseconds value (A fraction value from 10-12 digits length)
+    Picoseconds(u64),
+    /// Parsed femtoseconds value (A fraction value from 12-15 digits length)
+    Femtoseconds(u64),
+    /// A parsed value truncated to nanoseconds (A fraction value of 15+ digits length)
+    Truncated(u64), // An unbound fraction value truncated to nanoseconds
+}


Suggestion: change this to

pub struct Fraction { fraction_digits: u8, value: u64, }

and then give it functions such as

impl Fraction { pub fn try_to_nanoseconds(self) -> Option<u64> { if self.fraction_digits <= 9 { // compute the pow and return } else { None } } pub fn to_truncated_nanoseconds(self) -> u64 { // compute pow, either + or - } }

Another future advantage of this model is that the Fraction can directly return a FixedDecimal, which could make fractional second formatting more efficient in icu4x.

components/timezone/src/ixdtf.rs

robertbastian · 2025-01-27T11:57:08Z

utils/ixdtf/src/error.rs

@@ -46,6 +46,8 @@ pub enum ParseError {
    TimeSecond,
    #[displaydoc("Invalid character while parsing fraction part value.")]
    FractionPart,
+    #[displaydoc("Fraction part value exceeds a representable range.")]
+    InvalidFractionRange,


"Invalid" is not very descriptive. This is an error, that already tells me that something is not valid.

Suggested change

InvalidFractionRange,

ExcessiveSecondPrecision,

I was double checking this and the error is specific to a Duration hour fraction and minute fraction potentially exceeding the range of u64::MAX when calculated. Maybe DurationFractionExceededRange (this would then be moved down into the Duration errors).

components/timezone/src/ixdtf.rs

utils/ixdtf/src/error.rs

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

sffc · 2025-02-16T20:02:23Z

utils/ixdtf/src/parsers/records.rs

+    pub fn to_nanoseconds(&self) -> Option<u32> {
+        if self.digits <= 9 {
+            10u32
+                .checked_pow(9 - u32::from(self.digits))
+                .map(|x| x * self.value as u32)
+        } else {
+            None
+        }
+    }
+
+    /// Returns a `u64` representing the `Fraction` as it's computed
+    /// nanosecond value, truncating any value beyond 9 digits to
+    /// nanoseconds
+    pub fn to_truncated_nanoseconds(&self) -> u64 {
+        if self.digits <= 9 {
+            self.value
+        } else {
+            self.value / 10u64.pow(u32::from(self.digits - 9))
+        }


Issue: Something here doesn't look right. The code for the self.digits <= 9 case should be the same in both functions I think. Please make sure this is tested.

Huh, that's true. I think when I initially wrote that, it was with the idea of truncating on all cases, but that doesn't align with the function name.

I've updated the behavior and added some doc tests and tests for potentially malformed Fractions since the fields are public.

sffc

Thought: kind-of wish the Fraction type didn't have so many weird invariants. It is also a bit sad that we need to do so much checked arithmetic to get the nanoseconds out of it. There's probably still room to iterate on its design.

Note: the TC decided to start using the language "subsecond" instead of "fractional second" in icu_datetime, but I don't know if that also applies to the ixdtf crate, and even if it does, it shouldn't block merging this PR.

nekevss · 2025-02-17T03:28:19Z

Agreed on the amount of checking that's needed for arithmetic. I wasn't super happy with it. It could probably be avoided by making Fraction non-exhaustive or just removed in favor of GIGO alongside some debug_asserts as most of the checks are primarily for somebody constructing a Fraction of their own. Most of the arithmetic checks would not affect Fractions output from parsing in ixdtf.

I'm more inclined to refer to this as Fraction as RFC3339 defines the fraction in ABNF as time-secfrac and refers to it elsewhere as "fractional second digits". At a quick glance, the language subsecond does not exist in either RFC3339 or RFC9557.

sffc · 2025-02-17T17:22:52Z

Maybe make Fraction have private fields so we can mess with the implementation in the future.

And maybe keep to_truncated_nanoseconds() returning a non-Option.

nekevss

Made some changes to Fraction by making the fields private (pub(crate)) and updating digits to be a NonZeroU8.

I think this is cleaner than the previous version.

General question: should there be getters for digit and value?

nekevss · 2025-02-18T00:34:50Z

components/time/src/ixdtf.rs

+            .map(|fraction| {
+                fraction
+                    .to_nanoseconds()
+                    .ok_or(ParseError::ExcessivePrecision)


The ExcessivePrecision error could be remove here in favor of calling to_truncated_nanoseconds if that were preferred.

Would also remove the transpose below.

robertbastian

Code looks good, however I'm not happy with the Fraction naming. I think Subsecond would be clearer, and it's a term we're introducing in icu_time and icu_datetime right now as well (#5999).

sffc · 2025-02-18T15:24:13Z

What terminology does the RFC use for fractional second / subsecond?

nekevss · 2025-02-18T15:51:33Z

Subsecond isn't a totally accurate term when applied to the TimeDurationRecord (which actually now that I think about it needs to be adjusted for these changes even more than they currently are). Also, I mentioned it above, the language Subsecond does not exist in any of the RFCs, which uses "fractional second digits". I don't disagree with the change being made in reference to #5999, but I do think the context here is a tad different.

Essentially the above would mean that "T1.5H" is parsed as "T" HOUR_VALUE "." SUBSECOND "H", which to me makes less sense than "T" HOUR_VALUE "." FRACTIONAL_DIGITS "H"

To be fair, the name could be changed to FractionalDigits to more align with the spec.

sffc · 2025-02-18T15:55:20Z

Good point about this field being used for things other than seconds in the ixdtf crate.

Update fraction to handle unbounded fraction length

9477c8e

nekevss requested a review from a team as a code owner January 25, 2025 06:15

nekevss changed the title ~~Update fraction to handle unbound fraction length~~ Update ixdtf to handle unbound fraction length Jan 25, 2025

Add support for Fraction to icu_timezone

f86c37c

nekevss requested review from robertbastian and sffc as code owners January 25, 2025 06:39

sffc reviewed Jan 27, 2025

View reviewed changes

Update fraction according to feedback

6192c3e

robertbastian reviewed Jan 27, 2025

View reviewed changes

nekevss added 3 commits January 27, 2025 16:39

Change error to be more descriptive

73ea171

Update error to ExcessivePrecision and add Time::try_from_time_record

f3bfb57

Merge branch 'main' into update-fraction-handling

bf313b0

nekevss requested review from sffc and robertbastian February 10, 2025 17:08

robertbastian reviewed Feb 11, 2025

View reviewed changes

components/timezone/src/ixdtf.rs Outdated Show resolved Hide resolved

components/timezone/src/ixdtf.rs Outdated Show resolved Hide resolved

utils/ixdtf/src/error.rs Outdated Show resolved Hide resolved

nekevss and others added 2 commits February 11, 2025 11:33

Update components/timezone/src/ixdtf.rs

9a03d3b

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Adjust error based on review feedback

468228a

nekevss requested a review from robertbastian February 11, 2025 22:29

Merge branch 'main' into update-fraction-handling

17dd98d

sffc reviewed Feb 16, 2025

View reviewed changes

nekevss added 2 commits February 16, 2025 15:49

Update nanosecond methods and add some tests and doctests

9e09a7a

cargo fmt

dd21910

sffc previously approved these changes Feb 17, 2025

View reviewed changes

Make Fraction fields pub(crate) and make digits nonzero

1411c31

nekevss dismissed sffc’s stale review via 1411c31 February 18, 2025 00:32

Update duration docs for fraction change

01734ea

nekevss commented Feb 18, 2025

View reviewed changes

nekevss requested a review from sffc February 18, 2025 00:39

robertbastian reviewed Feb 18, 2025

View reviewed changes

Remove duration preprocessing

4b3ba88

sffc approved these changes Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `ixdtf` to handle unbound fraction length #6036

Update `ixdtf` to handle unbound fraction length #6036

nekevss commented Jan 25, 2025

sffc Jan 27, 2025

robertbastian Jan 27, 2025

nekevss Jan 27, 2025

sffc Feb 16, 2025

nekevss Feb 16, 2025

sffc left a comment

nekevss commented Feb 17, 2025

sffc commented Feb 17, 2025

nekevss left a comment

nekevss Feb 18, 2025

robertbastian left a comment

sffc commented Feb 18, 2025

nekevss commented Feb 18, 2025

sffc commented Feb 18, 2025

Update ixdtf to handle unbound fraction length #6036

Are you sure you want to change the base?

Update ixdtf to handle unbound fraction length #6036

Conversation

nekevss commented Jan 25, 2025

sffc Jan 27, 2025

Choose a reason for hiding this comment

robertbastian Jan 27, 2025

Choose a reason for hiding this comment

nekevss Jan 27, 2025

Choose a reason for hiding this comment

sffc Feb 16, 2025

Choose a reason for hiding this comment

nekevss Feb 16, 2025

Choose a reason for hiding this comment

sffc left a comment

Choose a reason for hiding this comment

nekevss commented Feb 17, 2025

sffc commented Feb 17, 2025

nekevss left a comment

Choose a reason for hiding this comment

nekevss Feb 18, 2025

Choose a reason for hiding this comment

robertbastian left a comment

Choose a reason for hiding this comment

sffc commented Feb 18, 2025

nekevss commented Feb 18, 2025

sffc commented Feb 18, 2025

Update `ixdtf` to handle unbound fraction length #6036

Update `ixdtf` to handle unbound fraction length #6036