tustvold commented on code in PR #4546:
URL: https://github.com/apache/arrow-rs/pull/4546#discussion_r1269591805
##########
arrow-array/src/types.rs:
##########
@@ -350,6 +352,59 @@ impl ArrowTimestampType for TimestampNanosecondType {
}
}
+fn add_year_months<T: ArrowTimestampType>(
+ timestamp: <T as ArrowPrimitiveType>::Native,
+ delta: <IntervalYearMonthType as ArrowPrimitiveType>::Native,
+ tz: Tz,
+) -> Result<<T as ArrowPrimitiveType>::Native, ArrowError> {
+ let months = IntervalYearMonthType::to_months(delta);
+ let res = as_datetime_with_timezone::<T>(timestamp, tz)
+ .ok_or_else(|| ArrowError::ComputeError("Timestamp out of
range".to_string()))?;
+ let res = shift_months_datetime(res, months)
+ .ok_or_else(|| ArrowError::ComputeError("Timestamp out of
range".to_string()))?;
+ let res = res.naive_utc();
+ T::make_value(res)
+ .ok_or_else(|| ArrowError::ComputeError("Timestamp out of
range".to_string()))
Review Comment:
Given the error is always the same, perhaps this method could just return an
Option, and this error mapping be handled in the caller?
##########
arrow-arith/Cargo.toml:
##########
@@ -46,3 +46,4 @@ num = { version = "0.4", default-features = false, features =
["std"] }
[features]
simd = ["arrow-array/simd"]
+chrono-tz = ["arrow-array/chrono-tz"]
Review Comment:
Typically the way we have handled this is to put these integration style
tests in the top-level arrow, as opposed to introducing feature flags on child
crates
##########
arrow-array/src/types.rs:
##########
@@ -454,25 +469,13 @@ impl TimestampSecondType {
/// * `timestamp` - The date on which to perform the operation
/// * `delta` - The interval to add
pub fn subtract_day_time(
- timestamp: <TimestampSecondType as ArrowPrimitiveType>::Native,
+ timestamp: <Self as ArrowPrimitiveType>::Native,
delta: <IntervalDayTimeType as ArrowPrimitiveType>::Native,
- ) -> Result<<TimestampSecondType as ArrowPrimitiveType>::Native,
ArrowError> {
+ tz: Tz,
+ ) -> Result<<Self as ArrowPrimitiveType>::Native, ArrowError> {
let (days, ms) = IntervalDayTimeType::to_parts(delta);
- let res = NaiveDateTime::from_timestamp_opt(timestamp,
0).ok_or_else(|| {
- ArrowError::ComputeError("Timestamp out of range".to_string())
- })?;
- let res = res
- .checked_sub_signed(Duration::days(days as i64))
- .ok_or_else(|| {
- ArrowError::ComputeError("Timestamp out of range".to_string())
- })?;
- let res = res
- .checked_sub_signed(Duration::milliseconds(ms as i64))
- .ok_or_else(|| {
- ArrowError::ComputeError("Timestamp out of range".to_string())
- })?;
- TimestampSecondType::make_value(res)
- .ok_or_else(|| ArrowError::ComputeError("Timestamp out of
range".to_string()))
+ let delta = IntervalDayTimeType::make_value(-days, -ms);
Review Comment:
I'm not sure if it matters, but potentially this negation could overflow. In
practice I think this will always result in timestamp overflow anyway, so
perhaps this doesn't matter
--
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]
