Repository navigation
GH-47400: [C++] Support adding/subtracting intervals to/from timestamps - #53325
DeviousCardi wants to merge 4 commits into
Conversation
…mestamps
Add add, add_checked, subtract and subtract_checked kernels for
timestamp +/- {month_interval, day_time_interval, month_day_nano_interval},
with and without a timezone (add also accepts the interval first).
Months and days are calendar units applied to the local time of the
timestamp, while the sub-day part of the interval is a fixed duration,
following pandas.DateOffset. add applies months, days, then the sub-day
part; subtract applies the sub-day part first. Adding months clamps the
day of month. Ambiguous or nonexistent resulting local times return an
error, and the checked variants detect overflow.
Implicit temporal promotion is no longer applied in arithmetic dispatch
when an argument is an interval, so that e.g. date32 + interval is not
silently cast to a timestamp.
|
|
github-actions
Bot
added
Component: C++
Component: Python
Component: Documentation
labels
|
@DeviousCardi Thanks for the PR, pls use the AI disclosure checklist as given in the Arrow PR template. |
|
|
|
@Reranko05 Thanks for the reminder. The PR description now includes the template's "Was AI used for this PR?" checklist and a description of the AI assistance, just above the issue link. |
|
|
| const int64_t offset = localizer.OffsetAtSys(FloorDiv(t, kUnitsPerSecond)); | ||
| const int64_t local = Add(t, offset * kUnitsPerSecond, st); | ||
| int64_t day = FloorDiv(local, kUnitsPerDay); | ||
| const int64_t time_of_day = local - day * kUnitsPerDay; |
There was a problem hiding this comment.
The intermediate calendar values can overflow even when the final timestamp fits.
For timestamp[ns], adding one day to INT64_MIN causes signed overflow here. Subtracting one day from INT64_MIN + 86400000000000 should return INT64_MIN, but the multiplication below reports overflow. Nonzero timezone offsets cause similar failures near the bounds.
Please avoid these intermediate overflows and add kernel regression tests for both bounds, including a nonzero timezone offset.
There was a problem hiding this comment.
Addressed in 46a2ad2. Calendar arithmetic now splits the timestamp into day and time before applying the timezone offset, then applies the calendar and offset deltas to the original timestamp. That avoids constructing an out-of-range local or day-to-unit intermediate when the final UTC value fits. Added checked and unchecked kernel tests at both INT64 bounds for UTC and fixed ±05:30 offsets. The rebuilt temporal test target passes.
| // 2025-11-02 01:30 EST - 1 day = 2025-11-01 01:30 EDT | ||
| {"America/New_York", "2025-11-02T06:30:00", 0, 1, 0, "2025-11-01T05:30:00"}, | ||
| // Subtraction undoes the mixed addition above: 24 hours, then 1 day | ||
| {"America/New_York", "2025-03-10T13:30:00", 0, 1, 24 * kHour, |
There was a problem hiding this comment.
Could you add a paired test showing the difference between subtracting a mixed interval and adding its negative?
Starting at 2025-03-10 09:30 EDT:
- Subtracting 1 day + 24 hours gives 2025-03-08 09:30 EST.
- Adding -1 day -24 hours gives 2025-03-08 08:30 EST.
Please also show that subtracting one calendar day and adding -1 calendar day give the same result. This would make the component order and its effect across DST clear.
There was a problem hiding this comment.
Added the paired DST test in 46a2ad2. Starting at 2025-03-10 09:30 EDT, subtracting 1 calendar day plus 24 hours returns 2025-03-08 09:30 EST, while adding the negative mixed interval returns 08:30 EST. The test also confirms that subtracting one calendar day equals adding negative one day, across all timestamp units and both checked and unchecked kernels. The rebuilt temporal test target passes.
|
|
| // Reconstructing day * kUnitsPerDay first can overflow near INT64_MIN. | ||
| const int64_t day_delta = Subtract(day, original_local_day, st); | ||
| const int64_t offset_delta = Subtract(offset, new_offset, st); | ||
| const int64_t delta = Add(Multiply(day_delta, kUnitsPerDay, st), |
There was a problem hiding this comment.
The intermediate overflow is still there. Adding 106752 days to timestamp[ns](INT64_MIN) gives 763145224192, which fits, but add_checked returns overflow because this multiplication happens first.
The new tests cover the reported examples. Please also cover a calendar delta larger than int64 when the final timestamp still fits.
There was a problem hiding this comment.
Fixed in 66a6b80. AddCalendar no longer adds day_delta * kUnitsPerDay to the original timestamp. It now rebuilds the result from the shifted day and the UTC time of day, and for negative days it multiplies (day + 1) and adds (time_of_day - kUnitsPerDay), so day * kUnitsPerDay never has to fit on its own. The tz lookup's local-seconds computation uses the same helper. All steps still use the checked helpers, so a result that doesn't fit is still reported as overflow. INT64_MIN ns + 106752 days now returns 763145224192 in both add and add_checked.
New tests in TestTemporalTimestampAndIntervalErrors use the smallest day count longer than INT64_MAX in the unit (106752 for ns, 106751992 for us), with no tz, UTC, +05:30 and -05:30, as both day_time and month_day_nano, in checked and unchecked kernels: INT64_MIN + n days, interval + INT64_MIN, the subtract back to INT64_MIN, and the mirror cases from INT64_MAX. A month case also covers 3509 months from INT64_MIN and back from INT64_MAX in ns. A delta of 2 * 106752 days still raises overflow in add_checked/subtract_checked, and the unchecked kernels wrap. The new tests fail on 46a2ad2 and pass now. arrow-compute-scalar-temporal-test and arrow-compute-scalar-math-test both pass.
Rebuild the result from the shifted day and time of day instead of adding days * units_per_day to the original timestamp, so that a calendar delta longer than int64 in the timestamp unit does not overflow when the final timestamp fits. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
|
| // Timezone transitions have second precision; the subsecond remainder | ||
| // does not affect which offset applies. | ||
| const int64_t local_seconds = | ||
| ComposeDays(day, time_of_day / kUnitsPerSecond, 86400, st); |
There was a problem hiding this comment.
The local-seconds calculation can still overflow when the final UTC result fits, no?
For timestamp[s], add_checked(INT64_MAX - 86400, one_day) returns INT64_MAX with +05:00, but overflow with equivalent Etc/GMT-5. The reverse case fails at INT64_MIN with Etc/GMT+5.
How should we handle that?
Keep local calendar time as a day and seconds-of-day pair for timezone lookup. Near the int64-second limits, use the terminal UTC offset without materializing local seconds or overflowing the timezone library offset arithmetic. The recomposed UTC timestamp still determines checked overflow. Add named and numeric offset regressions at both bounds across all timestamp units and day interval types, including results one unit outside the valid range. Assisted-by: OpenAI Codex (GPT-6.1) Signed-off-by: Aarav Jadav <aaravsjadav@gmail.com>
|
|
Rationale for this change
There is currently no kernel for
timestamp + interval, so e.g. "add one day to a zoned timestamp, keeping the wall clock time" requires a round trip through pandas (see #47400).What changes are included in this PR?
New kernels for
add,add_checked,subtractandsubtract_checked:timestamp(unit, tz) ± month_interval / day_time_interval / month_day_nano_interval -> timestamp(unit, tz), for all four units, with or without a timezoneadd/add_checkedalso accept the interval as the first argumentSemantics follow the discussion in the issue (Pandas'
DateOffset):2025-03-08 09:30 EST + 1 day = 2025-03-09 09:30 EDT.day_time_interval, nanoseconds of amonth_day_nano_interval) is a fixed duration, so2025-03-08 09:30 EST + 24h = 2025-03-09 10:30 EDT.addapplies months, then days, then the sub-day part.subtractapplies the sub-day part first, so subtracting "1 day 24 hours" undoes adding it (the mixed-interval examples in the issue). Months are applied before days in both directions, as inDateOffsetand PostgreSQL.Invaliderror is returned, with the same messages as the existingZonedLocalizer::ConvertLocalToSys. This is also what pandas does for a tz-awareSeries/DatetimeIndex+DateOffset(a scalarTimestampsilently resolves instead). The issue doesn't settle this point. If wanted, a follow-up could add an option to chooseearliest/latest/shift-forward likeAssumeTimezoneOptions.timestamp[s]) returnsInvalid._checkedvariants returnInvalidon int64 overflow; the unchecked variants wrap around like the other arithmetic kernels.Calendar arithmetic uses 64-bit year arithmetic, so the full range of
timestamp[s]is supported. The timezone lookups use the existingLocateZone/time_zone::get_infomachinery fromtemporal_internal.h.ArithmeticFunction::DispatchBestno longer applies implicit temporal promotion when one of the arguments is an interval. Otherwisedate32 + intervalwould be silently cast totimestamp[s] + interval. Dates are not supported yet; I'd suggest doing them in a follow-up once we agree on whatdate + interval with a sub-day partshould return.Are these changes tested?
Yes:
scalar_temporal_test.cc): month-end clamping, leap years, negative intervals, nulls, all units and all three interval types, both argument orders. DST transitions are covered in America/New_York, Europe/London and Australia/Lord_Howe (30 min DST), including results that land just after a gap or overlap and results that fall inside one (errors), plus a fixed-offset timezone. Error tests cover sub-unit and overflow cases. The expected DST values were checked against pandas 3.0.6.test_compute.py): parity withpandas.DateOffset+Timedeltaacross timezones and units, and the same DST gap/overlap errors as pandas.int64bounds, with and without fixed offsets, including calendar deltas whose length in the timestamp unit exceedsint64while the result still fits (e.g.INT64_MINns + 106752 days), month deltas near the bounds, and real overflow still raising in the_checkedkernels.Are there any user-facing changes?
Yes, new kernels for
add/subtract(documented incompute.rst). Existing kernels are unchanged.Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by:
I used an AI coding assistant (Claude) for this PR. It drafted most of the kernel code, the C++ and Python tests, and the docs paragraph. I reviewed all of it, and the expected DST values in the tests were checked against pandas independently of the kernel. The 64-bit calendar helpers (
DaysFromCivil/CivilFromDays) follow Howard Hinnant's public-domain date algorithms, which are linked in a comment.The named-timezone boundary follow-up used OpenAI Codex (GPT-6.1) for the implementation and C++ regression tests; this follow-up was reviewed and tested by Codex.