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

Timestamp is not round-trip equivalent with Display and parse() #15

Closed
chachi opened this issue Jun 24, 2024 · 2 comments · Fixed by #16
Closed

Timestamp is not round-trip equivalent with Display and parse() #15

chachi opened this issue Jun 24, 2024 · 2 comments · Fixed by #16

Comments

@chachi
Copy link

chachi commented Jun 24, 2024

The + 1 at the end of the impl From<Duration> breaks stringifying and then parsing the same number back out.

NTP64((secs << 32) + ((nanos * FRAC_PER_SEC) / NANO_PER_SEC) + 1)

The following test fails:

    #[test]
    fn test_exact_parse() {
        use std::str::FromStr;
        let id1: ID = ID::try_from([0x01]).unwrap();
        let ts1 = Timestamp::new(NTP64(100), id1);
        assert_eq!(ts1, Timestamp::from_str(&ts1.to_string()).unwrap());
    }
@chachi
Copy link
Author

chachi commented Jun 24, 2024

Hmm on further investigation, this may be due to the nanosecond rounding more than the +1

@chachi chachi changed the title +1 in conversion from Duration means it's not round-trip equivalent Timestamp is not round-trip equivalent with Display and parse() Jun 24, 2024
@JEnoch
Copy link
Member

JEnoch commented Jun 24, 2024

Right, the root cause is the rounding.

The Display trait and thus the ToString trait convert the NTP64 to a RFC3339 representation to be more "human readable".
As the last 32 bits of a NTP64 is the fraction of 1 second, converting this fraction to any subdivision of the second requires to make a division by 2^32. This means that 1u64 as NTP64 represents approximately 0,232830644 nanoseconds.

Thus converting a NTP64 to string and then back to NTP64 will give back the same number in only for ~23,28% of the numbers.
e.g. this test succeeds:

    #[test]
    fn test_exact_parse() {
        use std::str::FromStr;
        let id1: ID = ID::try_from([0x01]).unwrap();
        let ts1 = Timestamp::new(NTP64(104), id1);
        assert_eq!(ts1, Timestamp::from_str(&ts1.to_string()).unwrap());
    }

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 a pull request may close this issue.

2 participants