fix: time ± interval returns a wrapped time instead of an interval#23279
fix: time ± interval returns a wrapped time instead of an interval#23279vismaytiwari wants to merge 3 commits into
time ± interval returns a wrapped time instead of an interval#23279Conversation
github-actions
Bot
added
logical-expr
`time + interval`, `interval + time`, and `time - interval` previously widened the time operand into an interval, so `time '23:30' + interval '2 hours'` returned a `25 hours 30 mins` interval rather than wrapping within the 24-hour clock. Following the existing `Date - Date` special case, coercion now keeps the result as `time`, and a new `apply_time_interval` kernel adds/subtracts the interval's sub-day component modulo 24 hours to match PostgreSQL and DuckDB. Whole months and days are ignored, and all four time units are supported. Closes apache#22265
vismaytiwari
force-pushed
the
time-interval-returns-time
branch
from
0a33c81 to
5b6be8b
Compare
|
Fixed the rustfmt failure from the first run — fmt, the unit tests, and the datetime sqllogictests all pass locally. The workflows just need approval to run on the updated commit. |
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
it makes sense to make this change to me, given two other systems behave similarly and having time + interval = interval seems non-obvious (our current behaviour)
we'll need a note in the upgrade guide since this is a user facing change
| let time_ns = time_ns_arr.as_primitive::<Time64NanosecondType>(); | ||
| let interval_arr = cast( | ||
| interval_array, | ||
| &DataType::Interval(IntervalUnit::MonthDayNano), |
There was a problem hiding this comment.
this cast to monthdaynano should be unnecessary as logical layer coerces to the right type
There was a problem hiding this comment.
Good catch — dropped this one. The coercion already normalizes the interval to MonthDayNano, so we can downcast directly here.
|
|
||
| let left = lhs.to_array(num_rows)?; | ||
| let right = rhs.to_array(num_rows)?; | ||
|
|
There was a problem hiding this comment.
we have some optimization opportunity here to operate directly on columnarvalue instead of converting to arrays; see apply_date_subtraction for reference
There was a problem hiding this comment.
Done — reworked it to match on ColumnarValue directly (following apply_date_subtraction), so the scalar paths no longer materialize a full array. Thanks for the pointer.
| // Normalize to a single representation: time as Time64(ns), interval as MonthDayNano. | ||
| let time_ns_arr = cast(time_array, &DataType::Time64(TimeUnit::Nanosecond))?; |
There was a problem hiding this comment.
similarly here i feel we can avoid this cast
There was a problem hiding this comment.
Agreed. I moved the normalization up into the coercion layer, so the time operand now arrives as Time64(Nanosecond) and there's no cast here anymore. That does make the result Time64(Nanosecond) regardless of the input time unit — I called that out in the upgrade guide.
|
|
||
| let wrapped: Time64NanosecondArray = | ||
| arrow::compute::binary(time_ns, interval, |t, iv| { | ||
| let delta = iv.nanoseconds as i128; |
There was a problem hiding this comment.
why is operating on i128 necessary here?
There was a problem hiding this comment.
It isn't — switched to i64. Reducing the interval's nanoseconds modulo a day first keeps the sum in range, and rem_euclid handles the wrap, so i128 isn't needed.
- Coerce the operands of `time ± interval` to Time64(Nanosecond) and Interval(MonthDayNano) in the type coercion layer, so the physical layer no longer casts either operand. The result type is Time64(Nanosecond). - Rewrite apply_time_interval to operate directly on ColumnarValue (like apply_date_subtraction) instead of materializing arrays, and use i64 arithmetic with rem_euclid rather than i128. - Note the user-facing behavior change in the 55.0.0 upgrade guide and cover array/null inputs in the sqllogictest.
github-actions
Bot
added
the
documentation
2 participants
Which issue does this PR close?
time + intervalshould wrap within the 24-hour time domain #22265time - intervalshould wrap within the 24-hour time domain #22255Rationale for this change
time + interval(andinterval + time/time - interval) returned anIntervalinstead of atime, so the value was never wrapped within the 24-hour clock:PostgreSQL (and DuckDB) return a
timethat wraps around midnight:The root cause is in type coercion:
time <op> intervalwas coerced by widening thetimeoperand into anInterval, so the addition happened between two intervals and the result kept theIntervaltype.What changes are included in this PR?
Following the existing
Date - Datespecial case (in the same two files),time ± intervalis now handled explicitly:expr-common/src/type_coercion/binary.rs):time + interval,interval + time, andtime - intervalnow coerce to(time, interval)with atimeresult type (the interval is normalized toMonthDayNano).interval - time, which is not meaningful, is left unchanged.physical-expr/src/expressions/binary.rs): a newapply_time_intervaladds/subtracts the interval's sub-day component and wraps the result modulo 24 hours. All four time units (Time32(Second|Millisecond),Time64(Microsecond|Nanosecond)) and both operand orders are supported. Only the interval'snanosecondsaffect a time-of-day; whole months and days are ignored, matching PostgreSQL (e.g.time '10:00' + interval '1 day 2 hours'=12:00:00).time - time(→Interval) is unchanged.Are these changes tested?
Yes.
datafusion/sqllogictest/test_files/datetime/arith_time_interval.sltwas previously a characterization test that documented the incorrectIntervaloutput; it now asserts the correct wrappedtimeresults — including wrapping past midnight in both directions (22:00 + 3h → 01:00:00,02:00 - 3h → 23:00:00) and ignoring whole days.Are there any user-facing changes?
Yes —
time ± intervalnow returns atimevalue (wrapped within 24 hours) instead of anInterval, aligning DataFusion with PostgreSQL and DuckDB.