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

Update ixdtf to handle unbound fraction length #6036

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

nekevss
Copy link
Contributor

@nekevss nekevss commented Jan 25, 2025

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

@nekevss nekevss requested a review from a team as a code owner January 25, 2025 06:15
@nekevss nekevss changed the title Update fraction to handle unbound fraction length Update ixdtf to handle unbound fraction length Jan 25, 2025
Comment on lines 221 to 230
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
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
components/timezone/src/ixdtf.rs Outdated Show resolved Hide resolved
@@ -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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
InvalidFractionRange,
ExcessiveSecondPrecision,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants