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



##########
File path: arrow/Cargo.toml
##########
@@ -50,6 +50,7 @@ regex = "1.3"
 lazy_static = "1.4"
 packed_simd = { version = "0.3", optional = true, package = "packed_simd_2" }
 chrono = "0.4"
+chronoutil = "0.2"

Review comment:
       since `chronoutil` appears to only used in tests, perhaps we could put 
it in `dev-dependencies` instead

##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -166,8 +168,62 @@ where
     Ok(b.finish())
 }
 
+/// Add the given `time_delta` to each time in the `date_time` array.
+///
+/// # Errors
+/// If the arrays of different lengths.
+pub fn add_time<DateTime, TimeDelta>(
+    date_time: &PrimitiveArray<DateTime>,
+    time_delta: &PrimitiveArray<TimeDelta>,
+) -> Result<PrimitiveArray<DateTime>>
+where
+    DateTime: ArrowTimestampType,

Review comment:
       `ArrowTimestampType` type seems to only allow timestamp calculations, 
not for example adding a `Date32` and a `Duration`
   
   https://docs.rs/arrow/5.1.0/arrow/datatypes/trait.ArrowTimestampType.html 
   
   I wonder if it would make sense to define a `ArrowDateTimeType` or something 
that was implemented for all the timestamp types as well as `Date32`, `Time32`, 
etc and then use `ArrowDateTimeType` instead of `ArrowTimestampType` here

##########
File path: arrow/src/datatypes/types.rs
##########
@@ -161,25 +169,106 @@ impl ArrowTemporalType for DurationNanosecondType {}
 pub trait ArrowTimestampType: ArrowTemporalType {
     /// Returns the `TimeUnit` of this timestamp.
     fn get_time_unit() -> TimeUnit;
+
+    /// Return the `chrono::NavieDateTime` from the given value.
+    fn to_datetime(value: Self::Native) -> NaiveDateTime;
+
+    fn from_datetime(datetime: NaiveDateTime) -> Self::Native;
 }
 
 impl ArrowTimestampType for TimestampSecondType {
     fn get_time_unit() -> TimeUnit {
         TimeUnit::Second
     }
+
+    fn to_datetime(value: Self::Native) -> NaiveDateTime {
+        timestamp_s_to_datetime(value)
+    }
+
+    fn from_datetime(datetime: NaiveDateTime) -> Self::Native {
+        datetime.timestamp()
+    }
 }
 impl ArrowTimestampType for TimestampMillisecondType {
     fn get_time_unit() -> TimeUnit {
         TimeUnit::Millisecond
     }
+
+    fn to_datetime(value: Self::Native) -> NaiveDateTime {
+        timestamp_ms_to_datetime(value)
+    }
+
+    fn from_datetime(datetime: NaiveDateTime) -> Self::Native {
+        datetime.timestamp_millis()
+    }
 }
 impl ArrowTimestampType for TimestampMicrosecondType {
     fn get_time_unit() -> TimeUnit {
         TimeUnit::Microsecond
     }
+
+    fn to_datetime(value: Self::Native) -> NaiveDateTime {
+        timestamp_us_to_datetime(value)
+    }
+
+    fn from_datetime(datetime: NaiveDateTime) -> Self::Native {
+        datetime.timestamp_nanos() / 1000
+    }
 }
 impl ArrowTimestampType for TimestampNanosecondType {
     fn get_time_unit() -> TimeUnit {
         TimeUnit::Nanosecond
     }
+
+    fn to_datetime(value: Self::Native) -> NaiveDateTime {
+        timestamp_ns_to_datetime(value)
+    }
+
+    fn from_datetime(datetime: NaiveDateTime) -> Self::Native {
+        datetime.timestamp_nanos()
+    }
+}
+
+pub trait ArrowTimeDeltaType: ArrowPrimitiveType {

Review comment:
       👍 




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