-
Notifications
You must be signed in to change notification settings - Fork 721
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
allow using custom datetime format for inference and parsing csv file #1112
Conversation
9d01b17
to
af441b8
Compare
Codecov Report
@@ Coverage Diff @@
## master #1112 +/- ##
==========================================
- Coverage 82.34% 82.28% -0.07%
==========================================
Files 168 168
Lines 49479 49670 +191
==========================================
+ Hits 40743 40870 +127
- Misses 8736 8800 +64
Continue to review full report at Codecov.
|
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.
Thank you for the contribution @sum12 !
} | ||
DataType::Date64 => { | ||
build_primitive_array::<Date64Type>(line_number, rows, i) | ||
build_primitive_array::<Date32Type>(line_number, rows, i, None) |
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.
should this also have datetime_format
?
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.
this would need a date_format
.
Since datetime_re
/DATETIME_RE
regex rejected the string. Then using the same format for date32 and date64 can lead to parsing errors
line_number, | ||
rows, | ||
i, | ||
None, |
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.
datefime_format
?
line_number, | ||
rows, | ||
i, | ||
None, |
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.
also here?
af441b8
to
856cf39
Compare
The patch extends the current implementation to allow passing a custom datetime_re and datetime_format to the ReaderBuilder. datetime_re is used infer schema of the csv and then datetime_format is used to parse the actual string to a Date64. ofcourse passing non-compatible datetime_re and datetime_format values is going to fail the parsing or inference, however it is an expected but hard-to-detect failure.
The patch adds a new struct to collect all these options together and then passes the struct around. Ideally the struct could be embedded into the reader but that can be done as separate exercise.
The patch decides on using NaiveDateTime or DateTime from chrono lib based on presence of timezone components chrono expects timezone to be presetn if DateTime is used, errors otherwise. Whereas NaiveDateTime ignores timezone even if explicitly provided.
856cf39
to
2e91790
Compare
use chrono::format::Fixed; | ||
use chrono::format::StrftimeItems; | ||
let fmt = StrftimeItems::new(format); | ||
let has_zone = fmt.into_iter().any(|item| match item { |
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 would love to avoid evaluation of has_zone
on very call. Not sure if I can have some "attributes" on this particular implementation of Parser/Date64Type and avoid having to re-evaluate it everytime
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.
That is a cool idea -- I wonder if we could implement Date64TypeWithZone
or something -- or move the has_zone
attribute into Date64Type
directly?
escape: Option<u8>, | ||
quote: Option<u8>, | ||
terminator: Option<u8>, | ||
roptions: ReaderOptions, |
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.
👍 this is much nicer
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.
Thanks @sum12 -- I think this one is ready to go 👍
The patch extends the current implementation to allow passing a custom
datetime_re and datetime_format to the ReaderBuilder.
datetime_re is used infer schema of the csv and then datetime_format is
used to parse the actual string to a Date64.
ofcourse passing non-compatible datetime_re and datetime_format values
is going to fail the parsing or inference, however it is an expected but
hard-to-detect failure.
Which issue does this PR close?
Closes #.
Rationale for this change
Sometimes csv files follow non-standard datetime format. Arrow does not really care for the datetime format stored in the csv but merely guesses/assumes it to be of a certain fomrat. The change extends the csv reader to use a custom fromat.
What changes are included in this PR?
Are there any user-facing changes?
Any external libraries relying on ReaderBuilder and Reader structs will have to be updated to the new new API,