Parquet Derive: remove obscure feature flags, make chrono time emit converted type #712
Conversation
Codecov Report
@@ Coverage Diff @@
## master #712 +/- ##
=======================================
Coverage 82.46% 82.47%
=======================================
Files 168 168
Lines 47419 47461 +42
=======================================
+ Hits 39104 39143 +39
- Misses 8315 8318 +3
Continue to review full report at Codecov.
|
|
@nevi-me it seems like you've taken to proc macro hacking in here. perhaps this is something you could review? |
There was a problem hiding this comment.
Looks reasonable to me 👍 . Thank you @xrl
Since this PR is removing some feature flags, I think strictly speaking it is not backwards compatible (and thus we wouldn't backport to the 5.3 release (planned for next week) and instead include it in the 6.0 release (planned for a few months from now). Depending on your timeframe if you want to split this PR up so the feature flag removal is done in a separate PR we could get it into 5.3 I think
| @@ -534,13 +545,23 @@ impl Type { | |||
| })) } | |||
| } | |||
| "NaiveDate" => quote! { Some(LogicalType::DATE(Default::default())) }, | |||
| "NaiveDateTime" => quote! { None }, | |||
There was a problem hiding this comment.
I double checked and I agree there does not seem to be any DATETIME logical type in https://sourcegraph.com/github.com/apache/arrow-rs/-/blob/parquet/src/basic.rs?L166
|
@xrl let me know if it would be helpful to try and backport this into the arrow 5.x releases (e.g. arrow 5.4.0 ) |
|
@alamb thanks for the merge! I was just coming back to tackle a split up. I would love to get this core functionality in to |
* NaiveDateTime emits converted type
* NaiveDateTime emits converted type
* Parquet Derive: make chrono time emit converted type (#712) * NaiveDateTime emits converted type * restore over-zealously removed chrono dependency Co-authored-by: Xavier Lange <[email protected]>
Which issue does this PR close?
Closes #711
Rationale for this change
I could not upgrade parquet-rs for my derive-heavy project.
What changes are included in this PR?
TIMESTAMP_MILLISAre there any user-facing changes?
I have tested it on my code base and it works without changes. Users may want to remove the feature flags in the Cargo.toml.