[GitHub] [arrow-rs] alamb commented on a change in pull request #651: rough draft of time arithmetic

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

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



##########
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:
       > The alternative would be to have fewer limitations at the type level and instead make it a runtime error to pass an invalid time type
   
   This is my personal preference. The rationale being:
   1. Any code that uses this will likely be dynamically switching on types anyways (as the kernels would get `&dyn Array`)
   2. The overhead of a runtime check once per pair of arrays is likely quite small compared to the actual calculation being performed
   3. Encoding logic in trait bounds, while powerful, makes the code harder to use, especially for those relatively new to Rust (see, for example, [`tower::Service`](https://docs.rs/tower/0.4.11/tower/trait.Service.html)) -- given the runtime overhead is likely so small, I don't think the complexity is worth it.




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