Add max_dyn and min_dyn for max/min for dictionary array #2585
Conversation
|
cc @sunchao |
| @@ -215,6 +215,68 @@ where | |||
| } | |||
| } | |||
|
|
|||
| /// Returns the min of values in the array. | |||
| pub fn min_dyn<T, A: ArrayAccessor<Item = T::Native>>(array: A) -> Option<T::Native> | |||
There was a problem hiding this comment.
Typically the _dyn kernels take a trait object, these don't appear to. Perhaps we could choose a different name?
There was a problem hiding this comment.
I wonder if we can't just have a single min kernel for all types? We currently have separate implementations for strings, primitives, etc... which is no longer needed?
There was a problem hiding this comment.
I guess that we have separate min for different type of arrays because there was no common accessor previously.
But the min for primitive array, it has simd and non simd versions. I'm hesitant to replace them with this ArrayAccessor version. Do you think it is okay?
There was a problem hiding this comment.
I think lets press forward with this PR as is, but renamed to min_array, and I'll write up a ticket to look into this
There was a problem hiding this comment.
Renamed to min_array, max_array and sum_array
| ) | ||
| } | ||
|
|
||
| fn min_max_dyn_helper<T, A: ArrayAccessor<Item = T::Native>, F, M>( |
There was a problem hiding this comment.
| fn min_max_dyn_helper<T, A: ArrayAccessor<Item = T::Native>, F, M>( | |
| fn min_max_array_helper<T, A: ArrayAccessor<Item = T::Native>, F, M>( |
|
Benchmark runs are scheduled for baseline = 8eea918 and contender = c64ca4f. c64ca4f 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 #2584.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?