alamb commented on code in PR #4038:
URL: https://github.com/apache/arrow-rs/pull/4038#discussion_r1167214146
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -3665,4 +3863,588 @@ mod tests {
"1234567890.0000000000000000000000000000"
);
}
+
+ #[test]
+ fn test_timestamp_second_add_interval() {
+ // timestamp second + interval year month
+ let a = TimestampSecondArray::from(vec![1, 2, 3, 4, 5]);
+ let b = IntervalYearMonthArray::from(vec![
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
Review Comment:
It would be good if at least one of these intervals had a year field set to
makes sure it survive the roundtrip
Like
```
Some(IntervalYearMonthType::make_value(1, 2)),
```
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -739,6 +739,105 @@ pub fn add_dyn(left: &dyn Array, right: &dyn Array) ->
Result<ArrayRef, ArrowErr
))),
}
}
+ DataType::Timestamp(TimeUnit::Second, _) => {
Review Comment:
One thing I noticed while reviewing this code is that it will only support
`Timestamp + Interval` not `Interval + Timestamp` - given this seems to be the
case for `Date + Interval` as well, I don't think we have to fix it as part of
this PR, but I will file a follow on ticket to try and improve the situation.
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -3665,4 +3863,588 @@ mod tests {
"1234567890.0000000000000000000000000000"
);
}
+
+ #[test]
+ fn test_timestamp_second_add_interval() {
+ // timestamp second + interval year month
+ let a = TimestampSecondArray::from(vec![1, 2, 3, 4, 5]);
+ let b = IntervalYearMonthArray::from(vec![
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ ]);
+
+ let result = add_dyn(&a, &b).unwrap();
+ let result = result
+ .as_any()
+ .downcast_ref::<TimestampSecondArray>()
+ .unwrap();
Review Comment:
FYI you can write this more concisely like
```suggestion
let result = result.as_primitive::<TimestampSecondType>();
```
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -739,6 +739,105 @@ pub fn add_dyn(left: &dyn Array, right: &dyn Array) ->
Result<ArrayRef, ArrowErr
))),
}
}
+ DataType::Timestamp(TimeUnit::Second, _) => {
+ let l = left.as_primitive::<TimestampSecondType>();
+ match right.data_type() {
+ DataType::Interval(IntervalUnit::YearMonth) => {
+ let r = right.as_primitive::<IntervalYearMonthType>();
+ let res = math_checked_op(l, r,
TimestampSecondType::add_year_months)?;
+ Ok(Arc::new(res))
Review Comment:
@Weijun-H do you plan to make the change suggested by @metesynnada ? it
seems a good one .
##########
arrow-arith/src/arithmetic.rs:
##########
@@ -3665,4 +3863,588 @@ mod tests {
"1234567890.0000000000000000000000000000"
);
}
+
+ #[test]
+ fn test_timestamp_second_add_interval() {
+ // timestamp second + interval year month
+ let a = TimestampSecondArray::from(vec![1, 2, 3, 4, 5]);
+ let b = IntervalYearMonthArray::from(vec![
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
+ Some(IntervalYearMonthType::make_value(0, 1)),
Review Comment:
Likewise in all the tests below, it would be nice if more than one field was
non zero so that we can be sure the field values are passed through to the
corresponding operation
##########
arrow-array/src/types.rs:
##########
@@ -350,6 +350,650 @@ impl ArrowTimestampType for TimestampNanosecondType {
}
}
+impl TimestampSecondType {
+ /// Adds the given IntervalYearMonthType to an arrow TimestampSecondType
+ ///
+ /// # Arguments
+ ///
+ /// * `timestamp` - The date on which to perform the operation
+ /// * `delta` - The interval to add
+ pub fn add_year_months(
+ timestamp: <TimestampSecondType as ArrowPrimitiveType>::Native,
+ delta: <IntervalYearMonthType as ArrowPrimitiveType>::Native,
Review Comment:
I tried to come up with a better formulation of this signature to avoid the
boiler plate repetition (by finding some way to make the timestamp types have a
templated `to_naive` function but I couldn't come up with anything better.
--
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]
