[GitHub] [arrow-rs] alamb commented on a change in pull request #1376: Add extract week in temporal.rs

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

alamb commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817006099



##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
         assert_eq!(44, b.value(2));
     }
 
+    #[test]
+    fn test_temporal_array_date32_week() {
+        let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+        let b = week(&a).unwrap();
+        assert_eq!(1, b.value(0));
+        assert!(!b.is_valid(1));
+        assert_eq!(2, b.value(2));
+    }
+
+    #[test]
+    fn test_temporal_array_date64_week() {
+        // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+        // 1640998800000 -> 2022.01.01
+        let a: PrimitiveArray<Date64Type> = vec![
+            Some(1646116175000),
+            None,
+            Some(1641171600000),
+            Some(1640998800000),
+        ]
+        .into();
+
+        let b = week(&a).unwrap();
+        assert_eq!(9, b.value(0));
+        assert!(!b.is_valid(1));
+        assert_eq!(1, b.value(2));
+        assert_eq!(52, b.value(3));

Review comment:
       I don't understand how a date of `2022.01.01` would have a week of 52. I wonder if the comments on what date `1640998800000` corresponds to is incorrect?

##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -197,6 +211,24 @@ where
     Ok(b.finish())
 }
 
+/// Extracts the week of a given temporal array as an array of integers
+pub fn week<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+where
+    T: ArrowTemporalType + ArrowNumericType,
+    i64: std::convert::From<T::Native>,
+{
+    let mut b = Int32Builder::new(array.len());
+
+    match array.data_type() {
+        &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => {

Review comment:
       I agree it is important (eventually) to handle the `timezone` 
   
   This function seems to follow the model of existing functions such as `second` which also don't handle `timezone`
   
   I suggest we merge this PR in as is (without timezone support) and file a ticket for proper handling of timezone support for all the extract functions

##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
         assert_eq!(44, b.value(2));
     }
 
+    #[test]
+    fn test_temporal_array_date32_week() {
+        let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+        let b = week(&a).unwrap();
+        assert_eq!(1, b.value(0));
+        assert!(!b.is_valid(1));
+        assert_eq!(2, b.value(2));
+    }
+
+    #[test]
+    fn test_temporal_array_date64_week() {
+        // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+        // 1640998800000 -> 2022.01.01
+        let a: PrimitiveArray<Date64Type> = vec![
+            Some(1646116175000),
+            None,
+            Some(1641171600000),
+            Some(1640998800000),
+        ]
+        .into();
+
+        let b = week(&a).unwrap();
+        assert_eq!(9, b.value(0));
+        assert!(!b.is_valid(1));
+        assert_eq!(1, b.value(2));
+        assert_eq!(52, b.value(3));
+    }
+
+    #[test]
+    fn test_temporal_array_timestamp_micro_week() {
+        let a: TimestampMicrosecondArray =

Review comment:
       could you possibly also comment here about what dates are present?




-- 
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