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


##########
arrow-arith/src/numeric.rs:
##########
@@ -1402,16 +1402,13 @@ mod tests {
         test_duration_impl::<DurationNanosecondType>();
     }
 
-    fn test_date_impl<T: ArrowPrimitiveType, F>(f: F)
-    where
-        F: Fn(NaiveDate) -> T::Native,
-        T::Native: TryInto<i64>,
-    {
-        let a = PrimitiveArray::<T>::new(
+    #[test]
+    fn test_date64_impl() {

Review Comment:
   maybe this should just be called `test_date64` now as it only tests date 64



##########
arrow-arith/src/numeric.rs:
##########
@@ -1448,9 +1441,9 @@ mod tests {
         assert_eq!(result.as_ref(), &a);
 
         let b = IntervalDayTimeArray::from(vec![
-            IntervalDayTimeType::make_value(34, 2),
-            IntervalDayTimeType::make_value(3, -3),
-            IntervalDayTimeType::make_value(-12, 4),
+            IntervalDayTimeType::make_value(34, 0),

Review Comment:
   Why are these changes needed? Would it be possible to update the expected 
output too? Or is the issue the display looks nasty with millisecond precision 
too?
   
   I see the same values are used in the tests below as well



##########
arrow-arith/src/numeric.rs:
##########
@@ -1520,4 +1499,94 @@ mod tests {
             "Arithmetic overflow: Overflow happened on: 9223372036854775807 - 
-1"
         );
     }
+
+    #[test]
+    fn test_date32_impl() {

Review Comment:
   likewise this might make sense to call `test_date32`



##########
arrow-array/src/types.rs:
##########
@@ -1035,6 +1035,16 @@ impl Date64Type {
         epoch.add(Duration::try_milliseconds(i).unwrap())
     }
 
+    /// Converts an arrow Date64Type into a chrono::NaiveDateTime
+    ///
+    /// # Arguments
+    ///
+    /// * `i` - The Date64Type to convert
+    pub fn to_naive_datetime(i: <Date64Type as ArrowPrimitiveType>::Native) -> 
NaiveDateTime {
+        let datetime = NaiveDateTime::default();
+        datetime.add(Duration::try_milliseconds(i).unwrap())

Review Comment:
   Can we also add some documentation about when this will panic? Or comments 
explaining why it won't panic 🤔 



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

Reply via email to