-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Use lexical-core
for performance / accuracy
#53
Comments
+1 on this, specifically, I think we should get these tests working: #[test]
fn can_parse_4295391417_429607() {
let dt = DateTime::parse_str("4295391417.429607").unwrap();
assert_eq!(dt.time.microsecond, 429607);
}
#[test]
fn can_parse_1719776042_999989() {
DateTime::parse_str("1719776042.999989").unwrap();
} We got these working in #71, but reverted the changes to float parsing, knowing that we'll want to go with this approach in the long term. |
Also: #[test]
fn can_accurately_parse_recent_timestamps() {
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}")
}
let recently_us = 1719776043_000000;
for delta in 0..1_000_000 {
let us = recently_us - delta;
assert_eq!(roundtrip_us(us), Ok(us));
}
} |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In #51 we looked at using
lexical-core
in one specific case and decided instead to go with #52 for simplicity for now.However in principle
lexical-core
should be more accurate for float parsing, which seems worth moving forward with sometime.Also we should try some benchmark using
lexical-core
for integers instead of our hand-rolled implementations innumbers.rs
; if it's similar (or hopefully better) then it's worth switching for integers too.The text was updated successfully, but these errors were encountered: