diff --git a/datafusion/expr-common/src/type_coercion/binary.rs b/datafusion/expr-common/src/type_coercion/binary.rs index 7842b25aa8f9a..349e0ae93badf 100644 --- a/datafusion/expr-common/src/type_coercion/binary.rs +++ b/datafusion/expr-common/src/type_coercion/binary.rs @@ -267,6 +267,22 @@ impl<'a> BinaryTypeCoercer<'a> { ret: Int64, }); } + Plus | Minus if is_time_interval_arithmetic(lhs, rhs, self.op) => { + // `time ± interval` yields a `time` wrapped within the 24-hour clock, + // matching PostgreSQL and DuckDB (e.g. `time '23:30' + interval '2 hours'` + // is `01:30:00`). Both operands are normalized so the physical layer needs + // no casting: the time side to `Time64(Nanosecond)` and the interval side + // to `MonthDayNano`. The result is that wrapped `Time64(Nanosecond)`. + let (lhs, rhs) = match (lhs, rhs) { + (Interval(_), _) => (Interval(MonthDayNano), Time64(Nanosecond)), + (_, _) => (Time64(Nanosecond), Interval(MonthDayNano)), + }; + return Ok(Signature { + lhs, + rhs, + ret: Time64(Nanosecond), + }); + } Plus | Minus | Multiply | Divide | Modulo => { if let Ok(ret) = self.get_result(lhs, rhs) { @@ -362,6 +378,23 @@ fn is_date_minus_date(lhs: &DataType, rhs: &DataType) -> bool { ) } +/// Returns true for `time + interval`, `interval + time`, or `time - interval`. +/// +/// These follow PostgreSQL/DuckDB semantics where the result is a `time` value +/// wrapped within the 24-hour clock, rather than being widened to an interval. +fn is_time_interval_arithmetic(lhs: &DataType, rhs: &DataType, op: &Operator) -> bool { + use DataType::{Interval, Time32, Time64}; + match op { + Operator::Plus => matches!( + (lhs, rhs), + (Time32(_) | Time64(_), Interval(_)) | (Interval(_), Time32(_) | Time64(_)) + ), + // `interval - time` is not meaningful, so only `time - interval` is accepted. + Operator::Minus => matches!((lhs, rhs), (Time32(_) | Time64(_), Interval(_))), + _ => false, + } +} + /// Coercion rules for mathematics operators between decimal and non-decimal types. fn math_decimal_coercion( lhs_type: &DataType, diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index 6f0b60556a751..078f939eecde9 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -274,6 +274,129 @@ 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 `Time64(Nanosecond)` 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. +/// +/// The type coercion layer normalizes the operands to `Time64(Nanosecond)` and +/// `Interval(MonthDayNano)`, so no casting is needed 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, +) -> Result { + /// Nanoseconds in a 24-hour day. + const DAY_NANOS: i64 = 86_400_000_000_000; + + /// Wraps `time_ns ± interval.nanoseconds` into `[0, DAY_NANOS)`. The interval is + /// reduced modulo a day first so the addition stays within `i64`, and + /// `rem_euclid` keeps the result non-negative even when subtracting. + fn wrap(time_ns: i64, interval: IntervalMonthDayNano, subtract: bool) -> i64 { + let delta = interval.nanoseconds % DAY_NANOS; + let delta = if subtract { -delta } else { delta }; + (time_ns + delta).rem_euclid(DAY_NANOS) + } + + /// Extracts a `Time64(Nanosecond)` scalar as nanoseconds since midnight. + fn time_scalar_ns(scalar: &ScalarValue) -> Result> { + match scalar { + ScalarValue::Time64Nanosecond(value) => Ok(*value), + other => { + internal_err!( + "Time64(Nanosecond) scalar expected, got: {}", + other.data_type() + ) + } + } + } + + /// Extracts an `Interval(MonthDayNano)` scalar. + fn interval_scalar(scalar: &ScalarValue) -> Result> { + match scalar { + ScalarValue::IntervalMonthDayNano(value) => Ok(*value), + other => { + internal_err!( + "Interval(MonthDayNano) scalar expected, got: {}", + other.data_type() + ) + } + } + } + + // The `time` operand determines the result; the other is the interval. + let (time, interval) = if matches!(lhs.data_type(), DataType::Interval(_)) { + (rhs, lhs) + } else { + (lhs, rhs) + }; + + match (time, interval) { + (ColumnarValue::Array(time), ColumnarValue::Array(interval)) => { + let time = time.as_primitive::(); + let interval = interval.as_primitive::(); + let result: Time64NanosecondArray = + arrow::compute::binary(time, interval, |t, iv| wrap(t, iv, subtract))?; + Ok(ColumnarValue::Array(Arc::new(result))) + } + (ColumnarValue::Array(time), ColumnarValue::Scalar(interval)) => { + let time = time.as_primitive::(); + match interval_scalar(interval)? { + Some(iv) => { + let result: Time64NanosecondArray = + time.unary(|t| wrap(t, iv, subtract)); + Ok(ColumnarValue::Array(Arc::new(result))) + } + None => Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(None))), + } + } + (ColumnarValue::Scalar(time), ColumnarValue::Array(interval)) => { + let interval = interval.as_primitive::(); + match time_scalar_ns(time)? { + Some(t) => { + let result: Time64NanosecondArray = + interval.unary(|iv| wrap(t, iv, subtract)); + Ok(ColumnarValue::Array(Arc::new(result))) + } + None => Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(None))), + } + } + (ColumnarValue::Scalar(time), ColumnarValue::Scalar(interval)) => { + let result = time_scalar_ns(time)? + .zip(interval_scalar(interval)?) + .map(|(t, iv)| wrap(t, iv, subtract)); + Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(result))) + } + } +} + impl PhysicalExpr for BinaryExpr { fn data_type(&self, input_schema: &Schema) -> Result { BinaryTypeCoercer::new( @@ -356,6 +479,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); + } + Operator::Minus + if is_time_minus_interval(&left_data_type, &right_data_type) => + { + return apply_time_interval(&lhs, &rhs, true); + } 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) diff --git a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt index 997eae9b1bd8b..ee3cc02d67222 100644 --- a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt +++ b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt @@ -1,70 +1,102 @@ # 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 + +# Regardless of the input time unit, the result is normalized to Time64(ns). +query T +SELECT arrow_typeof(arrow_cast('22:00', 'Time32(Second)') + interval '3 hours') +---- +Time64(ns) -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 + +# Array inputs (not only scalars) exercise the columnar path, including nulls. +statement ok +CREATE TABLE time_vals(id INT, t TIME) AS VALUES (1, '01:00'::time), (2, '22:00'::time), (3, NULL); + +query D +SELECT t + interval '3 hours' FROM time_vals ORDER BY id +---- +04:00:00 +01:00:00 +NULL + +query D +SELECT t - interval '2 hours' FROM time_vals ORDER BY id +---- +23:00:00 +20:00:00 +NULL + +statement ok +DROP TABLE time_vals diff --git a/docs/source/library-user-guide/upgrading/55.0.0.md b/docs/source/library-user-guide/upgrading/55.0.0.md index 307de722bd13e..68ab945a18b0b 100644 --- a/docs/source/library-user-guide/upgrading/55.0.0.md +++ b/docs/source/library-user-guide/upgrading/55.0.0.md @@ -460,3 +460,21 @@ type aliases: Implement the newly introduced types for your custom cache implementation. See [PR #22613](https://github.com/apache/datafusion/pull/22613) for details. + +### `time ± interval` now returns a `time` instead of an `interval` + +Adding or subtracting an `interval` to/from a `time` value now returns a `time` +that wraps within the 24-hour clock, matching PostgreSQL and DuckDB. Previously +DataFusion returned an `interval`. + +```sql +-- 55.0.0 onwards: returns a Time64(Nanosecond) +SELECT time '23:30:00' + interval '2 hours'; +-- 01:30:00 +``` + +Only the sub-day portion of the interval affects the result; whole days and +months are ignored, as in PostgreSQL. The result type is always +`Time64(Nanosecond)`. + +See [PR #23279](https://github.com/apache/datafusion/pull/23279) for details.