arrow::compute::kernels::temporal should support nanoseconds
#2996
Conversation
|
@waitingkuo @tustvold please review the PR whenever you have time. |
| } | ||
|
|
||
| /// Extracts the seconds of a given temporal array as an array of integers | ||
| pub fn second_generic<T, A: ArrayAccessor<Item = T::Native>>( |
There was a problem hiding this comment.
I think we have lost this function?
| } | ||
|
|
||
| /// Extracts the seconds fraction of a given temporal array as an array of integers | ||
| pub fn second_fraction_generic<T, A: ArrayAccessor<Item = T::Native>, F>( |
There was a problem hiding this comment.
Perhaps we could call this something like unary_timestamp or something? I would also not make it public, so we can alter it down the line
There was a problem hiding this comment.
i suggest that we could either
- separate them (aka second_generic, and nano_generic), keep it consistent wither other operator
- make it more general, combines not only second, nanosecond. (e.g.
second_fraction_generic::<T, _, _>(array, "minute", |t| t.minute() as i32should work forminute)
| /// Extracts the seconds of a given temporal array as an array of integers | ||
| fn second_internal<T, A: ArrayAccessor<Item = T::Native>>( | ||
| /// Extracts the seconds fraction of a given temporal array as an array of integers | ||
| fn second_fraction_internal<T, A: ArrayAccessor<Item = T::Native>, F>( |
There was a problem hiding this comment.
It occurs to me that this function can probably be implemented more efficiently using try_unary, probably not something for this PR though
Edit: It actually can't because of the use of ArrayAccessor 😢
|
Thanks @tustvold for the feedback. There is no breaking change, I have made |
|
|
||
| #[test] | ||
| fn test_temporal_array_date64_nanosecond() { | ||
| // new Date(1667328721453) | ||
| // Tue Nov 01 2022 11:52:01 GMT-0700 (Pacific Daylight Time) | ||
| // | ||
| // new Date(1667328721453).getMilliseconds() | ||
| // 453 | ||
|
|
||
| let a: PrimitiveArray<Date64Type> = vec![None, Some(1667328721453)].into(); | ||
|
|
||
| let b = nanosecond(&a).unwrap(); | ||
| assert!(!b.is_valid(0)); | ||
| assert_eq!(453_000_000, b.value(1)); | ||
| } | ||
| } |
There was a problem hiding this comment.
👍
i think we could add another test case for test nanosecond() for Timestamp data type like TimestampSecondArray
|
@waitingkuo thanks for your feedback. Renamed functions to |
There was a problem hiding this comment.
This PR seems to now remove the public second_generic and minute_generic functions. This makes it a breaking API change as downstreams using these methods will no longer work without modification. Perhaps we could rollback this change?
I would like to remove these _generic kernels, and replace them with dyn variants like we have for other kernels, but I think we should do this as a separate PR for all of them - I will create a ticket flr this
Edit: created #3004 perhaps you might like to work on this?
| } | ||
|
|
||
| /// Extracts the time fraction of a given temporal array as an array of integers | ||
| pub fn time_fraction_generic<T, A: ArrayAccessor<Item = T::Native>, F>( |
There was a problem hiding this comment.
Made private. Returned public second_generic and minute_generic methods as well as tests for them.
|
Thank you |
|
Benchmark runs are scheduled for baseline = b1050b7 and contender = a2c6647. a2c6647 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2995.
Rationale for this change
arrow::compute::kernels::temporalshould support nanoseconds to let calculate nanos, millis, microsWhat changes are included in this PR?
Support nano seconds for temporal
Are there any user-facing changes?
Added public function
nanosecond