-
Notifications
You must be signed in to change notification settings - Fork 60
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
parser: sanitize timestamps to RFC3339 #1201
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
6a8b599
parser: sanitize timestamps to RFC3339
mdibaiee b4fed06
parser: support timestamps with arbitrary number of fractional seconds
mdibaiee edb1750
parser: add benchmark
mdibaiee 0f27b4c
parser: use time crate to parse, update benchmark to remove I/O
mdibaiee 15e5b90
parser: improve performance by first parsing a naive date
mdibaiee 28f9f4d
parser: refactor tests so it is easier to test different cases
mdibaiee File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
#[path="../tests/testutil.rs"] mod testutil; | ||
|
||
use criterion::{criterion_group, criterion_main, Criterion, black_box}; | ||
|
||
use parser::ParseConfig; | ||
use testutil::{input_for_file, run_parser}; | ||
|
||
fn peoples_500(c: &mut Criterion) { | ||
let path = "benches/data/people-500.csv"; | ||
let cfg = ParseConfig { | ||
filename: Some(path.to_string()), | ||
..Default::default() | ||
}; | ||
|
||
c.bench_function("peoples_500", |b| b.iter(|| { | ||
let input = input_for_file(path); | ||
run_parser(&cfg, input, false); | ||
})); | ||
} | ||
|
||
criterion_group!(benches, peoples_500); | ||
criterion_main!(benches); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
use crate::{ParseConfig, Output, format::ParseResult, ParseError}; | ||
use time::macros::format_description; | ||
use serde_json::Value; | ||
|
||
struct DatetimeSanitizer { | ||
from: Output, | ||
default_offset: time::UtcOffset, | ||
} | ||
|
||
// Here we are trying to parse non-RFC3339 dates | ||
fn datetime_to_rfc3339(val: &mut Value, default_offset: time::UtcOffset) { | ||
match val { | ||
Value::String(s) => { | ||
let primitive_format = format_description!( | ||
mdibaiee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
version = 2, | ||
"[year]-[month]-[day][optional [T]][optional [ ]][hour]:[minute]:[second][optional [.[subsecond]]][optional [Z]][optional [z]][optional [[offset_hour]:[offset_minute]]]" | ||
); | ||
|
||
let parsed_no_tz = time::PrimitiveDateTime::parse(&s, primitive_format).ok(); | ||
|
||
let parsed_with_tz = if parsed_no_tz.is_some() { | ||
let offset_format = format_description!( | ||
version = 2, | ||
"[first | ||
[[year]-[month]-[day] [hour]:[minute]:[second][optional [.[subsecond]]]Z] | ||
[[year]-[month]-[day] [hour]:[minute]:[second][optional [.[subsecond]]]z] | ||
[[year]-[month]-[day] [hour]:[minute]:[second][optional [.[subsecond]]][offset_hour]:[offset_minute]] | ||
]" | ||
); | ||
|
||
time::OffsetDateTime::parse(&s, offset_format).ok() | ||
} else { None }; | ||
|
||
if let Some(parsed) = parsed_with_tz { | ||
*s = parsed.format(&time::format_description::well_known::Rfc3339).unwrap(); | ||
} else if let Some(parsed) = parsed_no_tz { | ||
*s = parsed.assume_offset(default_offset).format(&time::format_description::well_known::Rfc3339).unwrap(); | ||
} | ||
} | ||
|
||
Value::Array(vec) => { | ||
vec.iter_mut().for_each(|item| { | ||
datetime_to_rfc3339(item, default_offset) | ||
}) | ||
} | ||
|
||
Value::Object(map) => { | ||
map.iter_mut().for_each(|(_k, v)| { | ||
datetime_to_rfc3339(v, default_offset) | ||
}) | ||
} | ||
|
||
_ => {} | ||
} | ||
} | ||
|
||
impl Iterator for DatetimeSanitizer { | ||
type Item = ParseResult; | ||
|
||
fn next(&mut self) -> Option<Self::Item> { | ||
let next = self.from.next()?; | ||
Some(match next { | ||
Ok(mut val) => { | ||
datetime_to_rfc3339(&mut val, self.default_offset); | ||
Ok(val) | ||
} | ||
Err(e) => { | ||
mdibaiee marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Err(ParseError::Parse(Box::new(e))) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
#[derive(Debug, thiserror::Error)] | ||
pub enum DatetimeSanitizeError { | ||
#[error("could not parse offset: {0}")] | ||
OffsetParseError(#[from] time::error::Parse), | ||
} | ||
|
||
pub fn sanitize_datetime(config: &ParseConfig, output: Output) -> Result<Output, DatetimeSanitizeError> { | ||
let offset = time::UtcOffset::parse(&config.default_offset, format_description!("[offset_hour]:[offset_minute]")).map_err(DatetimeSanitizeError::OffsetParseError)?; | ||
let sanitizer = DatetimeSanitizer { | ||
from: output, | ||
default_offset: offset, | ||
}; | ||
|
||
return Ok(Box::new(sanitizer)) | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
use crate::{ParseConfig, Output}; | ||
|
||
pub mod datetime; | ||
|
||
#[derive(Debug, thiserror::Error)] | ||
pub enum SanitizeError { | ||
#[error("sanitizing datetimes: {0}")] | ||
DatetimeSanitizeError(#[from] datetime::DatetimeSanitizeError), | ||
} | ||
|
||
pub fn sanitize_output(config: &ParseConfig, output: Output) -> Result<Output, SanitizeError> { | ||
datetime::sanitize_datetime(config, output).map_err(SanitizeError::DatetimeSanitizeError) | ||
} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
"no_timezone", "no_timezone_fractional", "no_t", "no_t_fractional", "no_t_large_fractional" | ||
"2020-01-01T00:00:00","2020-01-01T00:00:00.000","2020-01-01 00:00:00","2020-01-01 00:00:00.000","2020-01-01 00:00:00.000000000" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
{"x": ["2020-01-01 00:00:00"], "y": { "z": ["2020-01-01 00:00:00"], "k": "2020-01-01 00:00:00" } } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
"no_timezone", "no_timezone_fractional", "rfc3339", "timezone_offset", "no_t", "no_t_fractional", "no_t_fractional_large" | ||
"2020-01-01T00:00:00","2020-01-01T00:00:00.000","2020-01-01T00:00:00Z","2020-01-01 00:00:00+00:00","2020-01-01 00:00:00","2020-01-01 00:00:00.000","2020-01-01 00:00:00.000000000" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
mod testutil; | ||
|
||
use parser::ParseConfig; | ||
use testutil::{input_for_file, run_test}; | ||
|
||
#[test] | ||
fn sanitize_datetime_to_rfc3339() { | ||
let path = "tests/examples/datetimes.csv"; | ||
let cfg = ParseConfig { | ||
filename: Some(path.to_string()), | ||
..Default::default() | ||
}; | ||
|
||
let input = input_for_file(path); | ||
let output = run_test(&cfg, input); | ||
output.assert_success(1); | ||
|
||
let expected_first_row = "2020-01-01T00:00:00Z"; | ||
for value in output.parsed[0].as_object().unwrap().values() { | ||
assert_eq!(expected_first_row, value.as_str().unwrap()) | ||
} | ||
} | ||
|
||
#[test] | ||
fn sanitize_datetime_to_rfc3339_offset() { | ||
let path = "tests/examples/datetimes-naive.csv"; | ||
let cfg = ParseConfig { | ||
default_offset: "-05:00".to_string(), | ||
filename: Some(path.to_string()), | ||
..Default::default() | ||
}; | ||
|
||
let input = input_for_file(path); | ||
let output = run_test(&cfg, input); | ||
output.assert_success(1); | ||
|
||
let expected_first_row = "2020-01-01T00:00:00-05:00"; | ||
for value in output.parsed[0].as_object().unwrap().values() { | ||
assert_eq!(expected_first_row, value.as_str().unwrap()) | ||
} | ||
} | ||
|
||
#[test] | ||
fn sanitize_datetime_to_rfc3339_nested() { | ||
let path = "tests/examples/datetimes-nested.json"; | ||
let cfg = ParseConfig { | ||
filename: Some(path.to_string()), | ||
..Default::default() | ||
}; | ||
|
||
let input = input_for_file(path); | ||
let output = run_test(&cfg, input); | ||
output.assert_success(1); | ||
|
||
let expected = "2020-01-01T00:00:00Z"; | ||
let out = output.parsed[0].as_object().unwrap(); | ||
assert_eq!(expected, out.get("x").unwrap().as_array().unwrap()[0].as_str().unwrap()); | ||
assert_eq!(expected, out.get("y").unwrap().as_object().unwrap().get("z").unwrap().as_array().unwrap()[0].as_str().unwrap()); | ||
assert_eq!(expected, out.get("y").unwrap().as_object().unwrap().get("k").unwrap().as_str().unwrap()); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure exactly what we should do about this, but would like to point out that in the common case where a string isn't a timestamp, we're trying to parse it 6 times. If there's a way to cut down on that, it might be worth it.
One possibility might be to switch from
chrono
to thetime
crate, which has the ability to specify optional elements in the format specifier. Thetime
crate is generally preferred overchrono
anyway. We currently use both (chrono being used a bit more, actually), but I'd like us to gradually standardize on just usingtime
if we can. So it might be worthwhile to switch to time now, if it seems like it could significantly cut down on the amount of work we have to do here.All this is of course speculative without any sort of benchmarks. I just brought up the current lack of benchmarks after standup, and Johnny's suggestion was to just try a basic before and after tests against a big CSV, so we can at least ensure that this isn't regressing performance egregiously. I agree that seems like a good compromise to avoid blowing up the scope of this PR. And I think we can let that determine whether it's worth switching to the
time
crate. As long as performance hasn't gotten significantly worse, it's fine the way it is for now.