alexandreyc commented on code in PR #4546:
URL: https://github.com/apache/arrow-rs/pull/4546#discussion_r1269879790
##########
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:
The only way to overflow an `x: i32` with negation is when `x = i32::MIN =
-2_147_483_648`.
It should not be a problem for years, months or days because they would
overflow the timestamp anyway. However it can be problematic for milliseconds
and nanoseconds.
We can use checked negation for those operations and return an "Interval out
of range" error. The only downside I see is performance wise since this check
must be done for every array element.
What's your opinion?
--
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]
