Fix timezoned timestamp arithmetic #4546

alexandreyc · 2023-07-20T13:22:43Z

Which issue does this PR close?

Closes #4457.

Rationale for this change

Arithmetic on timezoned timestamps was wrong.

Are there any user-facing changes?

Yes.

Methods add_year_months, add_day_time, add_month_day_nano, subtract_year_months, subtract_day_time and subtract_month_day_nano on TimestampSecondType, TimestampMillisecondType, TimestampMicrosecondType and TimestampNanosecondType now take an additonal parameter tz: Tz as these operations are inherently timezone-dependent. Maybe there is a way to do things differently and not break the API. Feel free to suggest any idea.

Tests

I tested the results against PostgreSQL 14. Feel free to propose new test cases that you find interesting.

Here is the script to reproduce the results:

postgres=# create temporary table tests as (

    select
        (timestamp '1970-01-28 23:00:00' at time zone 'Europe/Paris') as datetime,
        (interval '0 year 1 month') as interval_year_month,
        (interval '0 day 0 millisecond') as interval_day_time,
        (interval '1 month 0 day 0 microsecond') as interval_month_day_nano

    union all

    select
        (timestamp '1970-01-01 00:00:00' at time zone 'Europe/Paris') as datetime,
        (interval '5 year 34 month') as interval_year_month,
        (interval '5 day 454000 millisecond') as interval_day_time,
        (interval '344 month 34 day -43000000 microsecond') as interval_month_day_nano

    union all

    select
        (timestamp '2010-04-01 04:00:20' at time zone 'Europe/Paris') as datetime,
        (interval '-2 year 4 month') as interval_year_month,
        (interval '-34 day 0 millisecond') as interval_day_time,
        (interval '-593 month -33 day 13000000 microsecond') as interval_month_day_nano

    union all

    select
        (timestamp '1960-01-30 04:23:20' at time zone 'Europe/Paris') as datetime,
        (interval '7 year -4 month') as interval_year_month,
        (interval '7 day -4000 millisecond') as interval_day_time,
        (interval '5 month 2 day 493000000 microsecond') as interval_month_day_nano

    union all

    select
        (timestamp '2023-03-25 14:00:00' at time zone 'Europe/Paris') as datetime,
        (interval '0 year 1 month') as interval_year_month,
        (interval '1 day 0 millisecond') as interval_day_time,
        (interval '1 month 0 day 0 microsecond') as interval_month_day_nano

);
SELECT 5
postgres=# select * from tests
postgres=# select * from tests;
        datetime        | interval_year_month | interval_day_time |       interval_month_day_nano
------------------------+---------------------+-------------------+--------------------------------------
 1970-01-28 23:00:00+01 | 1 mon               | 00:00:00          | 1 mon
 1970-01-01 00:00:00+01 | 7 years 10 mons     | 5 days 00:07:34   | 28 years 8 mons 34 days -00:00:43
 2010-04-01 04:00:20+02 | -1 years -8 mons    | -34 days          | -49 years -5 mons -33 days +00:00:13
 1960-01-30 04:23:20+01 | 6 years 8 mons      | 7 days -00:00:04  | 5 mons 2 days 00:08:13
 2023-03-25 14:00:00+01 | 1 mon               | 1 day             | 1 mon
(5 rows)

postgres=# select datetime + interval_year_month from tests;
        ?column?
------------------------
 1970-02-28 23:00:00+01
 1977-11-01 00:00:00+01
 2008-08-01 04:00:20+02
 1966-09-30 04:23:20+01
 2023-04-25 14:00:00+02
(5 rows)

postgres=# select (datetime + interval_year_month) - interval_year_month from tests;
        ?column?
------------------------
 1970-01-28 23:00:00+01
 1970-01-01 00:00:00+01
 2010-04-01 04:00:20+02
 1960-01-30 04:23:20+01
 2023-03-25 14:00:00+01
(5 rows)

postgres=# select datetime + interval_day_time from tests;
        ?column?
------------------------
 1970-01-28 23:00:00+01
 1970-01-06 00:07:34+01
 2010-02-26 04:00:20+01
 1960-02-06 04:23:16+01
 2023-03-26 14:00:00+02
(5 rows)

postgres=# select (datetime + interval_day_time) - interval_day_time from tests;
        ?column?
------------------------
 1970-01-28 23:00:00+01
 1970-01-01 00:00:00+01
 2010-04-01 04:00:20+02
 1960-01-30 04:23:20+01
 2023-03-25 14:00:00+01
(5 rows)

postgres=# select datetime + interval_month_day_nano from tests;
        ?column?
------------------------
 1970-02-28 23:00:00+01
 1998-10-04 23:59:17+02
 1960-09-29 04:00:33+01
 1960-07-02 04:31:33+01
 2023-04-25 14:00:00+02
(5 rows)

postgres=# select (datetime + interval_month_day_nano) - interval_month_day_nano from tests;
        ?column?
------------------------
 1970-01-28 23:00:00+01
 1970-01-02 00:00:00+01
 2010-04-02 04:00:20+02
 1960-01-31 04:23:20+01
 2023-03-25 14:00:00+01
(5 rows)

tustvold

This looks really nice, left some initial comments, will do a more thorough review as soon as I find time

tustvold · 2023-07-20T14:58:12Z

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


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

I will do this change once we've settled on the rest.

tustvold · 2023-07-20T14:59:17Z

arrow-array/src/types.rs

+        .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()))


Given the error is always the same, perhaps this method could just return an Option, and this error mapping be handled in the caller?

After the last commits, the error can now be of two kinds: timestamp or interval out of range.

I'm not sure if it's really important to be able to distinguish between those errors... If not we can definitely return an Option here (though we change the API even more).

I don't think distinguishing them is important

tustvold · 2023-07-20T15:00:15Z

arrow-array/src/types.rs

-            })?;
-        TimestampSecondType::make_value(res)
-            .ok_or_else(|| ArrowError::ComputeError("Timestamp out of range".to_string()))
+        let delta = IntervalDayTimeType::make_value(-days, -ms);


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

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?

Perhaps we could use subtraction instead of negation followed by addition?

I think that ultimately we cannot avoid a checked negation because chrono's Months::new accepts only an u32 and arrow's months are i32. Same thing for days.

I've added checked negation everywhere it's needed.

I may have missed a simple solution without checked ops... Feel free to suggest any idea!

You should be able to use a combination of abs_unsigned and using the sign to select between addition and subtraction to avoid overflow?

Right, I didn't know about unsigned_abs... Should be good now!

alexandreyc · 2023-07-25T15:41:41Z

I just pushed a commit that makes the code simpler and shorter (IMHO).

In summary, I moved all arithmetic methods to the TimestampOp trait and made this trait public. This introduces API changes because users now need to have the trait in scope in order to call these methods. I also slightly renamed some methods.

What's your opinion on this change?

tustvold · 2023-07-25T18:18:40Z

arrow-arith/src/numeric.rs

-                Self::add_year_months(left, right)
-            }
+/// Arithmetic trait for timestamp arrays
+pub trait TimestampOp: ArrowTimestampType + Sized {


I'm not a fan of making this trait public, as it is intended as an internal implementation detail of how the kernel chooses to implement arithmetic. I could definitely see it evolving in future to support vectorisation or some other extensions.

I think I would prefer to just expose the dyn kernels

tustvold · 2023-07-25T18:19:08Z

arrow-array/src/types.rs

@@ -350,650 +350,6 @@ impl ArrowTimestampType for TimestampNanosecondType {
    }
 }

-impl TimestampSecondType {


I think removing these methods may cause non-trivial downstream friction. Perhaps we could deprecate them as a first step instead of removing them?

I reverted the last commit to undo this change because I'm not sure where to put these methods if we deprecate them and don't make TimestampOp public... Maybe we can create another trait? I'm not sure it's worth it...

…aller

…pe name

This reverts commit 8ec4be4.

tustvold · 2023-07-26T15:37:47Z

Thank you this looks good to me, I will file a follow on PR to remove the chrono-tz feature

github-actions bot added the arrow Changes to the arrow crate label Jul 20, 2023

tustvold reviewed Jul 20, 2023

View reviewed changes

tustvold added the api-change Changes to the arrow API label Jul 20, 2023

tustvold reviewed Jul 25, 2023

View reviewed changes

alexandreyc added 15 commits July 26, 2023 13:06

Fix arithmetic for timezone-aware timestamp arrays

6688b4e

Remove debug test from issue

ec04ea0

Update to pass timezone by value instead of reference because it's sm…

a5ef09a

…aller

Use as_datetime_with_timezone instead of manual conversion

6d7c810

Add support for string tz, fix bug and refactor the whole thing

bcefb7a

Use checked arithmetic for months and days and use Self instead of ty…

2bf8acf

…pe name

Refactor tests

9aa05c0

Add DST test case

f2a3147

Add new tz parameter to docstring

9a9f1bc

Add checked negations and refactor substraction

89660d9

Add tests for interval overflow

bb557c2

Remove checked negation and use absolute unsigned instead

f7810bc

Use Option instead of Result for overflow errors

2d27c77

Move arithmetic functions to TimestampOp trait

ff468db

Revert "Move arithmetic functions to TimestampOp trait"

fa33bec

This reverts commit 8ec4be4.

alexandreyc force-pushed the fix-4457 branch from b7d306a to fa33bec Compare July 26, 2023 11:06

tustvold merged commit 0b75e8f into apache:master Jul 26, 2023
25 checks passed

tustvold mentioned this pull request Jul 30, 2023

Timestamp Interval Arithmetic Ignores Timezone #4457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix timezoned timestamp arithmetic #4546

Fix timezoned timestamp arithmetic #4546

alexandreyc commented Jul 20, 2023

tustvold left a comment

tustvold Jul 20, 2023

alexandreyc Jul 24, 2023

tustvold Jul 26, 2023

tustvold Jul 20, 2023

alexandreyc Jul 24, 2023

tustvold Jul 24, 2023

alexandreyc Jul 24, 2023

tustvold Jul 20, 2023

alexandreyc Jul 20, 2023

tustvold Jul 21, 2023

alexandreyc Jul 24, 2023

tustvold Jul 24, 2023

alexandreyc Jul 24, 2023

alexandreyc commented Jul 25, 2023 •

edited

tustvold Jul 25, 2023 •

edited

tustvold Jul 25, 2023 •

edited

alexandreyc Jul 26, 2023

tustvold commented Jul 26, 2023

Fix timezoned timestamp arithmetic #4546

Fix timezoned timestamp arithmetic #4546

Conversation

alexandreyc commented Jul 20, 2023

Which issue does this PR close?

Rationale for this change

Are there any user-facing changes?

Tests

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandreyc commented Jul 25, 2023 • edited

tustvold Jul 25, 2023 • edited

Choose a reason for hiding this comment

tustvold Jul 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Jul 26, 2023

alexandreyc commented Jul 25, 2023 •

edited

tustvold Jul 25, 2023 •

edited

tustvold Jul 25, 2023 •

edited