tustvold commented on code in PR #2623:
URL: https://github.com/apache/arrow-rs/pull/2623#discussion_r961346254
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -516,7 +832,7 @@ mod tests {
let a: PrimitiveArray<Date64Type> =
vec![Some(1514764800000), None, Some(1550636625000)].into();
- let b = hour(&a).unwrap();
+ let b = hour::<Date64Type, _>(&a).unwrap();
Review Comment:
This loss of automatic type hinting is a bit unfortunate, it feels like a
step-back ergonomically for the common case
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -286,35 +405,65 @@ where
/// Monday is encoded as `0`, Tuesday as `1`, etc.
///
/// See also [`num_days_from_sunday`] which starts at Sunday.
-pub fn num_days_from_monday<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+pub fn num_days_from_monday<T, A: ArrayAccessor<Item = T::Native>>(
+ array: A,
+) -> Result<Int32Array>
where
T: ArrowTemporalType + ArrowNumericType,
+ T::Native: ArrowNativeType,
+ i64: std::convert::From<T::Native>,
+{
+ match array.data_type().clone() {
+ DataType::Dictionary(_, value_type) => {
+ num_days_from_monday_internal::<T, A>(array,
value_type.as_ref().clone())
Review Comment:
Is this `clone` really necessary? Can't num_days_from_monday_internal take
`&DataType`
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -172,109 +173,227 @@ pub fn using_chrono_tz_and_utc_naive_date_time(
}
/// Extracts the hours of a given temporal array as an array of integers
-pub fn hour<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+pub fn hour<T, A: ArrayAccessor<Item = T::Native>>(array: A) ->
Result<Int32Array>
where
T: ArrowTemporalType + ArrowNumericType,
+ T::Native: ArrowNativeType,
+ i64: std::convert::From<T::Native>,
+{
+ match array.data_type().clone() {
+ DataType::Dictionary(_, value_type) => {
+ hour_internal::<T, A>(array, value_type.as_ref().clone())
+ }
+ dt => hour_internal::<T, A>(array, dt),
+ }
+}
+
+/// Extracts the hours of a given temporal array as an array of integers
+fn hour_internal<T, A: ArrayAccessor<Item = T::Native>>(
+ array: A,
+ dt: DataType,
+) -> Result<Int32Array>
+where
+ T: ArrowTemporalType + ArrowNumericType,
+ T::Native: ArrowNativeType,
i64: std::convert::From<T::Native>,
{
let mut b = Int32Builder::with_capacity(array.len());
- match array.data_type() {
- &DataType::Time32(_) | &DataType::Time64(_) => {
- extract_component_from_array!(array, b, hour, value_as_time, |h| h
as i32)
+ match dt {
+ DataType::Time32(_) | DataType::Time64(_) => {
+ let iter = ArrayIter::new(array);
+ extract_component_from_array!(
+ iter,
+ b,
+ hour,
+ |value| as_time::<T>(i64::from(value)),
+ |h| h as i32
+ );
}
- &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None)
=> {
- extract_component_from_array!(array, b, hour, value_as_datetime,
|h| h as i32)
+ DataType::Date32 | DataType::Date64 | DataType::Timestamp(_, None) => {
+ let iter = ArrayIter::new(array);
+ extract_component_from_array!(
+ iter,
+ b,
+ hour,
+ |value| as_datetime::<T>(i64::from(value)),
+ |h| h as i32
+ )
}
- &DataType::Timestamp(_, Some(ref tz)) => {
+ DataType::Timestamp(_, Some(tz)) => {
let mut scratch = Parsed::new();
+ let iter = ArrayIter::new(array);
extract_component_from_array!(
- array,
+ iter,
b,
hour,
- value_as_datetime_with_tz,
+ |value, tz| as_datetime::<T>(i64::from(value))
+ .map(|datetime| datetime + tz),
tz,
scratch,
+ |value| as_datetime::<T>(i64::from(value)),
|h| h as i32
)
}
- dt => return_compute_error_with!("hour does not support", dt),
+ _ => return_compute_error_with!("hour does not support",
array.data_type()),
}
Ok(b.finish())
}
/// Extracts the years of a given temporal array as an array of integers
-pub fn year<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+pub fn year<T, A: ArrayAccessor<Item = T::Native>>(array: A) ->
Result<Int32Array>
+where
+ T: ArrowTemporalType + ArrowNumericType,
+ T::Native: ArrowNativeType,
Review Comment:
I think this should be implied by the definition of `ArrowPrimitiveType`
##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -28,40 +29,40 @@ use chrono::format::{parse, Parsed};
use chrono::FixedOffset;
macro_rules! extract_component_from_array {
- ($array:ident, $builder:ident, $extract_fn:ident, $using:ident,
$convert:expr) => {
- for i in 0..$array.len() {
- if $array.is_null(i) {
- $builder.append_null();
- } else {
- match $array.$using(i) {
+ ($iter:ident, $builder:ident, $extract_fn:ident, $using:expr,
$convert:expr) => {
Review Comment:
I have a really hard time following this macro, I really wonder if we can't
replace it with generics or at the very least simplify it... Something for a
future PR I suspect
##########
arrow/src/compute/kernels/cast.rs:
##########
@@ -1502,21 +1503,37 @@ where
if let Some(tz) = tz {
let mut scratch = Parsed::new();
- // The macro calls `value_as_datetime_with_tz` on timestamp values of
the array.
+ // The macro calls `as_datetime` on timestamp values of the array.
// After applying timezone offset on the datatime, calling `to_string`
to get
// the strings.
+ let iter = ArrayIter::new(array);
extract_component_from_array!(
- array,
+ iter,
builder,
to_string,
- value_as_datetime_with_tz,
+ |value, tz| as_datetime::<T>(<i64 as From<
+ <T as ArrowPrimitiveType>::Native,
+ >>::from(value))
+ .map(|datetime| datetime + tz),
tz,
scratch,
+ |value| as_datetime::<T>(
+ <i64 as From<<T as ArrowPrimitiveType>::Native>>::from(value)
+ ),
|h| h
)
} else {
// No timezone available. Calling `to_string` on the datatime value
simply.
- extract_component_from_array!(array, builder, to_string,
value_as_datetime, |h| h)
+ let iter = ArrayIter::new(array);
Review Comment:
Why is this `ArrayIter` needed? The macro calls `into_iter`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
