[GitHub] [arrow-rs] alamb commented on a diff in pull request #2850: Replace complicated temporal macro with generic functions

[GitBox <gi...@apache.org>]

alamb commented on code in PR #2850:
URL: https://github.com/apache/arrow-rs/pull/2850#discussion_r993856462


##########
arrow/src/compute/kernels/temporal.rs:
##########
@@ -28,83 +28,111 @@ use chrono::format::strftime::StrftimeItems;
 use chrono::format::{parse, Parsed};
 use chrono::FixedOffset;
 
-macro_rules! extract_component_from_array {
-    ($iter:ident, $builder:ident, $extract_fn:ident, $using:expr, $convert:expr) => {
-        $iter.into_iter().for_each(|value| {
-            if let Some(value) = value {
-                match $using(value) {
-                    Some(dt) => $builder.append_value($convert(dt.$extract_fn())),
-                    None => $builder.append_null(),
-                }
-            } else {
-                $builder.append_null();
+fn as_time_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+    iter: ArrayIter<A>,
+    mut builder: PrimitiveBuilder<Int32Type>,
+    op: F,
+) -> Int32Array
+where
+    F: Fn(NaiveTime) -> i32,
+    i64: From<T::Native>,
+{
+    iter.into_iter().for_each(|value| {
+        if let Some(value) = value {
+            match as_time::<T>(i64::from(value)) {
+                Some(dt) => builder.append_value(op(dt)),
+                None => builder.append_null(),
             }
-        })
-    };
-    ($iter:ident, $builder:ident, $extract_fn1:ident, $extract_fn2:ident, $using:expr, $convert:expr) => {
-        $iter.into_iter().for_each(|value| {
-            if let Some(value) = value {
-                match $using(value) {
-                    Some(dt) => {
-                        $builder.append_value($convert(dt.$extract_fn1().$extract_fn2()));
-                    }
-                    None => $builder.append_null(),
-                }
-            } else {
-                $builder.append_null();
+        } else {
+            builder.append_null();
+        }
+    });
+
+    builder.finish()
+}
+
+fn as_datetime_with_op<A: ArrayAccessor<Item = T::Native>, T: ArrowTemporalType, F>(
+    iter: ArrayIter<A>,
+    mut builder: PrimitiveBuilder<Int32Type>,
+    op: F,
+) -> Int32Array
+where
+    F: Fn(NaiveDateTime) -> i32,
+    i64: From<T::Native>,
+{
+    iter.into_iter().for_each(|value| {
+        if let Some(value) = value {
+            match as_datetime::<T>(i64::from(value)) {
+                Some(dt) => builder.append_value(op(dt)),
+                None => builder.append_null(),
             }
-        })
-    };
-    ($iter:ident, $builder:ident, $extract_fn:ident, $using:expr, $tz:ident, $parsed:ident, $value_as_datetime:expr, $convert:expr) => {
-        if ($tz.starts_with('+') || $tz.starts_with('-')) && !$tz.contains(':') {
-            return_compute_error_with!(
-                "Invalid timezone",
-                "Expected format [+-]XX:XX".to_string()
-            )
         } else {
-            let tz_parse_result = parse(&mut $parsed, &$tz, StrftimeItems::new("%z"));
-            let fixed_offset_from_parsed = match tz_parse_result {
-                Ok(_) => match $parsed.to_fixed_offset() {
-                    Ok(fo) => Some(fo),
-                    err => return_compute_error_with!("Invalid timezone", err),
-                },
-                _ => None,
-            };
-
-            for value in $iter.into_iter() {
-                if let Some(value) = value {
-                    match $value_as_datetime(value) {
-                        Some(utc) => {
-                            let fixed_offset = match fixed_offset_from_parsed {
-                                Some(fo) => fo,
-                                None => match using_chrono_tz_and_utc_naive_date_time(
-                                    &$tz, utc,
-                                ) {
+            builder.append_null();
+        }
+    });
+
+    builder.finish()
+}
+
+fn extract_component_from_datatime_array<

Review Comment:
   It would help me to have some docstrings here that explained the parameters and types here
   
   However this is already much easier to read than the macro so 👍 



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org