-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: time ± interval returns a wrapped time instead of an interval
#23279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,6 +274,87 @@ where | |
| } | ||
| } | ||
|
|
||
| /// Returns true for `time + interval` or `interval + time`. | ||
| fn is_time_plus_interval(lhs: &DataType, rhs: &DataType) -> bool { | ||
| matches!( | ||
| (lhs, rhs), | ||
| ( | ||
| DataType::Time32(_) | DataType::Time64(_), | ||
| DataType::Interval(_) | ||
| ) | ( | ||
| DataType::Interval(_), | ||
| DataType::Time32(_) | DataType::Time64(_) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| /// Returns true for `time - interval`. | ||
| fn is_time_minus_interval(lhs: &DataType, rhs: &DataType) -> bool { | ||
| matches!( | ||
| (lhs, rhs), | ||
| ( | ||
| DataType::Time32(_) | DataType::Time64(_), | ||
| DataType::Interval(_) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| /// Evaluates `time + interval`, `interval + time`, or `time - interval`, returning | ||
| /// a `time` wrapped within the 24-hour clock to match PostgreSQL and DuckDB | ||
| /// (e.g. `time '23:30' + interval '2 hours'` is `01:30:00`). arrow's arithmetic | ||
| /// kernels do not implement time-of-day arithmetic, so it is handled here. | ||
| /// | ||
| /// Only the sub-day portion of the interval (its `nanoseconds`) affects a | ||
| /// time-of-day; whole months and days are ignored, matching PostgreSQL. | ||
| fn apply_time_interval( | ||
| lhs: &ColumnarValue, | ||
| rhs: &ColumnarValue, | ||
| subtract: bool, | ||
| num_rows: usize, | ||
| ) -> Result<ColumnarValue> { | ||
| /// Nanoseconds in a 24-hour day. | ||
| const DAY_NANOS: i128 = 86_400_000_000_000; | ||
|
|
||
| let left = lhs.to_array(num_rows)?; | ||
| let right = rhs.to_array(num_rows)?; | ||
|
|
||
| // The `time` operand determines the result type; the other is the interval. | ||
| let left_is_time = | ||
| matches!(left.data_type(), DataType::Time32(_) | DataType::Time64(_)); | ||
| let (time_array, interval_array) = if left_is_time { | ||
| (&left, &right) | ||
| } else { | ||
| (&right, &left) | ||
| }; | ||
| let time_type = time_array.data_type().clone(); | ||
|
|
||
| // Normalize to a single representation: time as Time64(ns), interval as MonthDayNano. | ||
| let time_ns_arr = cast(time_array, &DataType::Time64(TimeUnit::Nanosecond))?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similarly here i feel we can avoid this cast
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I moved the normalization up into the coercion layer, so the time operand now arrives as |
||
| let time_ns = time_ns_arr.as_primitive::<Time64NanosecondType>(); | ||
| let interval_arr = cast( | ||
| interval_array, | ||
| &DataType::Interval(IntervalUnit::MonthDayNano), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this cast to monthdaynano should be unnecessary as logical layer coerces to the right type
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch — dropped this one. The coercion already normalizes the interval to MonthDayNano, so we can downcast directly here. |
||
| )?; | ||
| let interval = interval_arr.as_primitive::<IntervalMonthDayNanoType>(); | ||
|
|
||
| let wrapped: Time64NanosecondArray = | ||
| arrow::compute::binary(time_ns, interval, |t, iv| { | ||
| let delta = iv.nanoseconds as i128; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is operating on
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It isn't — switched to |
||
| let total = if subtract { | ||
| t as i128 - delta | ||
| } else { | ||
| t as i128 + delta | ||
| }; | ||
| // Rust's `%` keeps the sign of the dividend, so add a day before the | ||
| // final modulo to always land in `[0, DAY_NANOS)`. | ||
| (((total % DAY_NANOS) + DAY_NANOS) % DAY_NANOS) as i64 | ||
| })?; | ||
|
|
||
| // Restore the original time unit (e.g. Time32(Second)). | ||
| let result = cast(&(Arc::new(wrapped) as ArrayRef), &time_type)?; | ||
| Ok(ColumnarValue::Array(result)) | ||
| } | ||
|
|
||
| impl PhysicalExpr for BinaryExpr { | ||
| fn data_type(&self, input_schema: &Schema) -> Result<DataType> { | ||
| BinaryTypeCoercer::new( | ||
|
|
@@ -356,6 +437,18 @@ impl PhysicalExpr for BinaryExpr { | |
| let input_schema = schema.as_ref(); | ||
|
|
||
| match self.op { | ||
| // `time ± interval` returns a wrapped `time` (PostgreSQL/DuckDB | ||
| // semantics); arrow's arithmetic kernels don't implement it. | ||
| Operator::Plus | ||
| if is_time_plus_interval(&left_data_type, &right_data_type) => | ||
| { | ||
| return apply_time_interval(&lhs, &rhs, false, batch.num_rows()); | ||
| } | ||
| Operator::Minus | ||
| if is_time_minus_interval(&left_data_type, &right_data_type) => | ||
| { | ||
| return apply_time_interval(&lhs, &rhs, true, batch.num_rows()); | ||
| } | ||
| Operator::Plus if self.fail_on_overflow => return apply(&lhs, &rhs, add), | ||
| Operator::Plus => return apply(&lhs, &rhs, add_wrapping), | ||
| // Special case: Date - Date returns Int64 (days difference) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,70 +1,75 @@ | ||
| # postgresql behavior | ||
| # | ||
| # time + interval → time | ||
| # Add an interval to a time | ||
| # Add an interval to a time. The result is a `time` value that wraps within the | ||
| # 24-hour clock, matching PostgreSQL and DuckDB. | ||
| # time '01:00' + interval '3 hours' → 04:00:00 | ||
| # | ||
| # note that while the above reflects what postgresql does | ||
| # in the case of datafusion/arrow that is not the case. The | ||
| # result will be an interval, not a time. | ||
| # time '22:00' + interval '3 hours' → 01:00:00 (wraps past midnight) | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT '01:00'::time + interval '3 hours' | ||
| ---- | ||
| 4 hours | ||
| 04:00:00 | ||
|
|
||
| query T | ||
| SELECT arrow_typeof('01:00'::time + interval '3 hours') | ||
| ---- | ||
| Interval(MonthDayNano) | ||
| Time64(ns) | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT '22:00'::time + interval '3 hours' | ||
| ---- | ||
| 25 hours | ||
| 01:00:00 | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT interval '3 hours' + '22:00'::time | ||
| ---- | ||
| 25 hours | ||
| 01:00:00 | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT arrow_cast('22:00', 'Time32(Second)') + interval '3 hours' | ||
| ---- | ||
| 25 hours | ||
| 01:00:00 | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT arrow_cast('22:00', 'Time32(Millisecond)') + interval '3 hours' | ||
| ---- | ||
| 25 hours | ||
| 01:00:00 | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT arrow_cast('22:00', 'Time64(Microsecond)') + interval '3 hours' | ||
| ---- | ||
| 25 hours | ||
| 01:00:00 | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT arrow_cast('22:00', 'Time64(Nanosecond)') + interval '3 hours' | ||
| ---- | ||
| 25 hours | ||
| 01:00:00 | ||
|
|
||
| # Whole days and months in the interval do not affect a time-of-day (PostgreSQL). | ||
| query D | ||
| SELECT '10:00'::time + interval '1 day 2 hours' | ||
| ---- | ||
| 12:00:00 | ||
|
|
||
| # postgresql behavior | ||
| # | ||
| # time - interval → time | ||
| # Subtract an interval from a time | ||
| # Subtract an interval from a time, wrapping within the 24-hour clock. | ||
| # time '05:00' - interval '2 hours' → 03:00:00 | ||
| # time '02:00' - interval '3 hours' → 23:00:00 (wraps before midnight) | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT '05:00'::time - interval '2 hours' | ||
| ---- | ||
| 3 hours | ||
| 03:00:00 | ||
|
|
||
| query T | ||
| SELECT arrow_typeof('05:00'::time - interval '2 hours') | ||
| ---- | ||
| Interval(MonthDayNano) | ||
| Time64(ns) | ||
|
|
||
| query ? | ||
| query D | ||
| SELECT '02:00'::time - interval '3 hours' | ||
| ---- | ||
| -1 hours | ||
| 23:00:00 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have some optimization opportunity here to operate directly on columnarvalue instead of converting to arrays; see
apply_date_subtractionfor referenceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — reworked it to match on
ColumnarValuedirectly (followingapply_date_subtraction), so the scalar paths no longer materialize a full array. Thanks for the pointer.