Simplify datetime conversion #5006
Conversation
tustvold
added
the
development-process
| @@ -427,13 +427,9 @@ impl Field { | |||
| quote! { ::chrono::naive::NaiveDateTime::from_timestamp_millis(vals[i]).unwrap() } | |||
| } | |||
| Some(ThirdPartyType::ChronoNaiveDate) => { | |||
| // NaiveDateTime::UNIX_EPOCH.num_days_from_ce() == 719163 | |||
There was a problem hiding this comment.
As this is a proc macro we would need to explicitly declare all the types and the Datelike trait. It seemed simpler to just define it
| ::chrono::naive::NaiveDate::from_num_days_from_ce_opt(vals[i] | ||
| + ((::chrono::naive::NaiveDate::from_ymd_opt(1970, 1, 1).unwrap() | ||
| .signed_duration_since( | ||
| ::chrono::naive::NaiveDate::from_ymd_opt(0, 12, 31).unwrap() |
There was a problem hiding this comment.
This caused me some confusion, but it is because days start counting at 1, not zero 😅
parquet_derive/src/parquet_field.rs
Outdated
| ::chrono::naive::NaiveDate::from_ymd_opt(0, 12, 31).unwrap() | ||
| ) | ||
| ).num_days()) as i32).unwrap() | ||
| ::chrono::naive::NaiveDate::from_num_days_from_ce_opt(vals[i] + 719163).unwrap() |
There was a problem hiding this comment.
Technically yes, practically no, unless you're handling dates in the year 11 million 😄
There was a problem hiding this comment.
so in debug builds (or w/ overflow checks) this panics when reading 3rd-party / untrusted input
There was a problem hiding this comment.
Correct, the implementation added in #4773 I would describe as an MVP
There was a problem hiding this comment.
ahhh, this is what it is for, interesting.
Either way, I would be happy if we don't introduce more panicky code paths. IIRC there's checked_add which pretty much solves the issue and the parquet error struct probably has some variant that would be fitting.
Which issue does this PR close?
Closes #.
Rationale for this change
Follow up to #4773
What changes are included in this PR?
Are there any user-facing changes?