Skip to content

Conversation

@jorgecarleitao
Copy link
Member

This is a small refactor that exposes the functions used to convert dates and times into NaiveDate, NaiveTime and NaiveDateTime. They are currently hidden beind the PrimitiveArray<ArrowTemporalType>::value_as_time, which makes them difficult to use when the transformation is to be applied in SIMD, or when we want to reproduce the operation for a single scalar value (not in an arrow array).

@github-actions
Copy link

@codecov-io
Copy link

codecov-io commented Jan 31, 2021

Codecov Report

Merging #9378 (6310c6d) into master (f05b49b) will increase coverage by 0.01%.
The diff coverage is 97.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9378      +/-   ##
==========================================
+ Coverage   81.94%   81.96%   +0.01%     
==========================================
  Files         231      231              
  Lines       53374    53372       -2     
==========================================
+ Hits        43739    43748       +9     
+ Misses       9635     9624      -11     
Impacted Files Coverage Δ
rust/arrow/src/array/array_primitive.rs 94.74% <91.66%> (-0.21%) ⬇️
rust/arrow/src/temporal_conversions.rs 100.00% <100.00%> (ø)
rust/parquet/src/encodings/encoding.rs 94.86% <0.00%> (-0.20%) ⬇️
rust/datafusion/src/physical_plan/hash_join.rs 83.52% <0.00%> (-0.15%) ⬇️
rust/datafusion/src/logical_plan/builder.rs 88.20% <0.00%> (-0.07%) ⬇️
rust/datafusion/src/optimizer/optimizer.rs
rust/datafusion/src/sql/planner.rs 84.17% <0.00%> (+0.01%) ⬆️
rust/datafusion/src/physical_plan/planner.rs 78.78% <0.00%> (+0.14%) ⬆️
rust/datafusion/src/execution/context.rs 89.21% <0.00%> (+0.49%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f05b49b...421ee98. Read the comment docs.

@Dandandan
Copy link
Contributor

This looks good 👍! This would also be helpful for removing some indirection in the temporal kernels. Maybe the module name could be called temporal_conversions or datetime_conversions?

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks @jorgecarleitao

@alamb
Copy link
Contributor

alamb commented Feb 1, 2021

I think this is small enough / not backwards breaking to merge. So doing so

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants