Also truncate negative milisecond fractions

[sydney-runkle]

This started as a fix for how we handle negative timestamps in the case of milliseconds, but I realized we were mishandling them in a couple of cases, so I expanded the scope of the fix and added corresponding tests.

Took a bit longer than expected, as I forgot how to do basic subtraction for a bit.

[davidhewitt]

I also think before we call from_timestamp_with_config we might need to adjust negative timestamps further by subtracting another [milli]second and then doing 1000[000] - fraction

[sydney-runkle]

I'll note, this is still a WIP, working on a better solution

[sydney-runkle]

The above change is also not right, just working through some ideas

[sydney-runkle]

Other notes:

It's challenging that we always use positive microseconds, but can have either negative or positive second values, resulting in ops like this:

pub(crate) fn timestamp_watershed(timestamp: i64) -> Result<(i64, u32), ParseError> {
        let ts_abs = timestamp.checked_abs().ok_or(ParseError::DateTooSmall)?;
        if ts_abs <= MS_WATERSHED {
            return Ok((timestamp, 0));
        }
        // if the timestamp is in milliseconds, then we need to convert it to seconds
        let mut seconds = timestamp / 1_000;
        let mut microseconds = ((timestamp % 1_000) * 1000) as i32;
        if microseconds < 0 {
            seconds -= 1;
            microseconds += 1_000_000;
        }
        Ok((seconds, microseconds as u32))
    }

[sydney-runkle]

It seems as though we have lots of bugs in our numeric timestamp processing...

[sydney-runkle]

This is ready for review - @samuelcolvin or @dmontagu, could you please take a look, given that @davidhewitt is out for a bit?

[latk]

Thank you for your work to handle timestamps correctly! The code looks very readable.

I did some tests that systematically searched for roundtrip problems. There are some scenarios where using floats as the intermediate representation leads to actual failures.

The function that I'm using to search for problems is the following, which formats a timestamp as a string and then parses it:

fn roundtrip_us(us: i64) -> Result<i64, ParseError> {
    let formatted_us = us.to_string();
    let formatted_s = reformat_us_to_s(&formatted_us); // insert decimal point
    let dt = DateTime::parse_str(&formatted_s)?;
    Ok(dt.timestamp() * 1_000_000 + i64::from(dt.time.microsecond))
}

fn reformat_us_to_s(us: &str) -> String {
    let (integral, fractional) = us.split_at(us.len() - 6);
    format!("{integral}.{fractional}")
}

Then I used tests like the following to systematically search for problematic examples (so basically a brute-force property-based test):

#[test]
fn can_accurately_parse_recent_timestamps() {
    let recently_us = 1719776043_000000;
    for delta in 0..1_000_000 {
        let us = recently_us - delta;
        assert_eq!(roundtrip_us(us), Ok(us));
    }
}

[sydney-runkle]

I did some tests that systematically searched for roundtrip problems. There are some scenarios where using floats as the intermediate representation leads to actual failures.

Thanks for sharing these tests! I've added them to the repo so that we can wait on merging until these are fixed.

[sydney-runkle]

I suppose we can wait to merge this into pydantic until v2.8.1, given that all of the changes in this most recent speedate release could arguably be categorized as fixes.

[sydney-runkle]

I've added an extra benchmark here, and here are some stats about the corresponding improvements:

on main

test parse_timestamp_str             ... bench:         310.35 ns/iter (+/- 3.57)

on my branch, before any performance optimizations:

test parse_timestamp_str             ... bench:         282.57 ns/iter (+/- 3.25)

on my branch, after moving the decimal digit counting to the existing parse bytes function:

test parse_timestamp_str             ... bench:         263.51 ns/iter (+/- 14.42)

Also, I locally broke these tests up individually, to confirm that the improvements were across the board:

mine:

running 11 tests
test parse_timestamp_str                       ... bench:         273.08 ns/iter (+/- 17.28)
test parse_timestamp_str_1654646400            ... bench:          22.03 ns/iter (+/- 1.43)
test parse_timestamp_str_1654646404            ... bench:          21.59 ns/iter (+/- 0.70)
test parse_timestamp_str_1654646404000_5       ... bench:          29.94 ns/iter (+/- 0.63)
test parse_timestamp_str_1654646404123_456     ... bench:          32.13 ns/iter (+/- 3.49)
test parse_timestamp_str_1654646404_123456     ... bench:          28.52 ns/iter (+/- 0.26)
test parse_timestamp_str_1654646404_5          ... bench:          25.59 ns/iter (+/- 0.67)
test parse_timestamp_str_neg_1654646400        ... bench:          21.83 ns/iter (+/- 1.18)
test parse_timestamp_str_neg_1654646404        ... bench:          22.46 ns/iter (+/- 1.55)
test parse_timestamp_str_neg_1654646404000_123 ... bench:          32.34 ns/iter (+/- 0.76)
test parse_timestamp_str_neg_1654646404_123456 ... bench:          29.40 ns/iter (+/- 92.68

main

running 11 tests
test parse_timestamp_str                       ... bench:         328.65 ns/iter (+/- 32.11)
test parse_timestamp_str_1654646400            ... bench:          24.99 ns/iter (+/- 1.11)
test parse_timestamp_str_1654646404            ... bench:          25.72 ns/iter (+/- 0.69)
test parse_timestamp_str_1654646404000_5       ... bench:          32.49 ns/iter (+/- 3.58)
test parse_timestamp_str_1654646404123_456     ... bench:          36.48 ns/iter (+/- 5.00)
test parse_timestamp_str_1654646404_123456     ... bench:          34.91 ns/iter (+/- 3.60)
test parse_timestamp_str_1654646404_5          ... bench:          28.86 ns/iter (+/- 1.13)
test parse_timestamp_str_neg_1654646400        ... bench:          24.76 ns/iter (+/- 1.29)
test parse_timestamp_str_neg_1654646404        ... bench:          36.21 ns/iter (+/- 46.17)
test parse_timestamp_str_neg_1654646404000_123 ... bench:          34.36 ns/iter (+/- 2.41)
test parse_timestamp_str_neg_1654646404_123456 ... bench:          34.00 ns/iter (+/- 0.36)

[sydney-runkle]

I guess the main question here is - is this a breaking change in terms of the API change for float_parse_bytes?

[sydney-runkle]

I guess the main question here is - is this a breaking change in terms of the API change for float_parse_bytes?

If so that's fine, I can revert and do the slightly less performant, but also ok approach where we basically parse the bytes twice.

[latk]

is this a breaking change in terms of the API change for float_parse_bytes?

It's definitely changing the public API in an incompatible way, so incrementing the major version would be appropriate for SemVer.

However, it would be extremely easy to add a compatibility layer to get the best of both worlds, efficient single-pass parsing and API stability:

1. rename the current float_parse_bytes() to something like float_parse_bytes_with_decimal_digit_length()

2. add a "new" compatibility layer that discards the extra length, roughly:

   #[inline(always)]
   pub fn float_parse_bytes(...) -> ... {
     let (int_or_float, _) = float_parse_bytes_with_decimal_digit_length(...);
     int_or_float
   }

[samuelcolvin]

It's definitely changing the public API in an incompatible way, so incrementing the major version would be appropriate for SemVer.

No. We haven't reach version 1, so there's no need to increment the major version.

[samuelcolvin]

Sorry, I fear there's more to change.

The issue with float parsing and the need to use lexical-parse-float has been weighing heavy on my conscience since we had to remove logic derived fro here in jiter and replace it with the crate.

[sydney-runkle]

I'm going to revert the changes to float parsing here (no breaking API changes, no modification of float parsing), given that we'll want to address #53 in the future.

[samuelcolvin]

LGTM

[latk]

I'm going to revert the changes to float parsing here (no breaking API changes, no modification of float parsing)

Bit disappointed that this got reverted. Yes, this had changed behavior, but in a good way.

I agree that parsing floats yourself is a fool's errand, which is why the decimal_integers idea was so good: parsing the timestamp as a decimal (fixed-point number that can be represented accurately as an integer), and then rescaling it once at the end. That avoided accumulating errors from repeatedly summing up small floats and helped to fix the bugs revealed by my roundtrip tests.

Because Speedate has a concept of a "precision overflow", it appears that the numbers being parsed here aren't really floats, but decimals with a limited number of significant base-10 digits. Ideally, this parsing code would just return an integer (to be interpreted on the ms or μs scale) rather than a float (to be interpreted on the s scale).

Switching to a fully-correct float parsing library (such as f64::from_str()) would of course also solve precision problems as long as Speedate has to use floats somewhere in its internal representation (which is acceptable for μs-precision timestamps), though in that case limiting the number of digits in the fractional part makes less sense (long float literals may round to short values).

I have a couple of hours I could throw at this problem. Would there be interest in me attempting to resurrect the decimal_integers approach, but without using floats as the intermediate representation? This should completely eliminate any concerns about precision or rounding modes during timestamp parsing (except for cases where the "precision overflow" error was disabled). My intention would be to write and test a hand-rolled parser function parse_decimal_timestamp_us(data: &[u8], watershed, config) -> Result<i64>.

[samuelcolvin]

It was my decision.

If we're parsing floats, we should use lexical-parse-float, in the mean time, I don't want to change our float parsing logic further. Happy to review a pr to use lexical-parse-float for float parsing.

If we can interpret decimals as integers, e.g. as microseconds, that's great, bit I don't think it's always possible without changing the internal presentation in speedate which I don't want to do unless it's absolutely necessary.