From 5b6be8b8fef4b9c6afeb6df69a9c81bbf6a7d1f4 Mon Sep 17 00:00:00 2001 From: vismaytiwari Date: Wed, 1 Jul 2026 17:56:29 +0530 Subject: [PATCH 1/4] =?UTF-8?q?fix:=20`time=20=C2=B1=20interval`=20returns?= =?UTF-8?q?=20a=20wrapped=20`time`=20instead=20of=20an=20interval?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #22265 --- .../expr-common/src/type_coercion/binary.rs | 32 +++++++ .../physical-expr/src/expressions/binary.rs | 93 +++++++++++++++++++ .../datetime/arith_time_interval.slt | 57 ++++++------ 3 files changed, 156 insertions(+), 26 deletions(-) diff --git a/datafusion/expr-common/src/type_coercion/binary.rs b/datafusion/expr-common/src/type_coercion/binary.rs index 7842b25aa8f9a..9043f19ca71cb 100644 --- a/datafusion/expr-common/src/type_coercion/binary.rs +++ b/datafusion/expr-common/src/type_coercion/binary.rs @@ -267,6 +267,21 @@ 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`). The interval is normalized to `MonthDayNano` so the + // physical layer only has to handle a single representation. + let (lhs, rhs, ret) = match (lhs, rhs) { + (Interval(_), time_type) => { + (Interval(MonthDayNano), time_type.clone(), time_type.clone()) + } + (time_type, _) => { + (time_type.clone(), Interval(MonthDayNano), time_type.clone()) + } + }; + return Ok(Signature { lhs, rhs, ret }); + } Plus | Minus | Multiply | Divide | Modulo => { if let Ok(ret) = self.get_result(lhs, rhs) { @@ -362,6 +377,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..3b5fd64c946eb 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -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 { + /// 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))?; + let time_ns = time_ns_arr.as_primitive::(); + let interval_arr = cast( + interval_array, + &DataType::Interval(IntervalUnit::MonthDayNano), + )?; + let interval = interval_arr.as_primitive::(); + + let wrapped: Time64NanosecondArray = + arrow::compute::binary(time_ns, interval, |t, iv| { + let delta = iv.nanoseconds as i128; + 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 { 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) diff --git a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt index 997eae9b1bd8b..0345acc3e37a9 100644 --- a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt +++ b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt @@ -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 From 4191b190d563441a057ad5bca943666a655d06ba Mon Sep 17 00:00:00 2001 From: vismaytiwari Date: Mon, 6 Jul 2026 10:31:47 +0530 Subject: [PATCH 2/4] Address review: canonicalize time/interval in coercion, avoid casts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../expr-common/src/type_coercion/binary.rs | 21 +-- .../physical-expr/src/expressions/binary.rs | 128 ++++++++++++------ .../datetime/arith_time_interval.slt | 27 ++++ .../library-user-guide/upgrading/55.0.0.md | 18 +++ 4 files changed, 141 insertions(+), 53 deletions(-) diff --git a/datafusion/expr-common/src/type_coercion/binary.rs b/datafusion/expr-common/src/type_coercion/binary.rs index 9043f19ca71cb..349e0ae93badf 100644 --- a/datafusion/expr-common/src/type_coercion/binary.rs +++ b/datafusion/expr-common/src/type_coercion/binary.rs @@ -270,17 +270,18 @@ impl<'a> BinaryTypeCoercer<'a> { 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`). The interval is normalized to `MonthDayNano` so the - // physical layer only has to handle a single representation. - let (lhs, rhs, ret) = match (lhs, rhs) { - (Interval(_), time_type) => { - (Interval(MonthDayNano), time_type.clone(), time_type.clone()) - } - (time_type, _) => { - (time_type.clone(), Interval(MonthDayNano), time_type.clone()) - } + // 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 }); + return Ok(Signature { + lhs, + rhs, + ret: Time64(Nanosecond), + }); } Plus | Minus | Multiply | Divide | Modulo => { if let Ok(ret) = self.get_result(lhs, rhs) { diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index 3b5fd64c946eb..078f939eecde9 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -300,59 +300,101 @@ fn is_time_minus_interval(lhs: &DataType, rhs: &DataType) -> bool { } /// 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. +/// 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. /// -/// Only the sub-day portion of the interval (its `nanoseconds`) affects a -/// time-of-day; whole months and days are ignored, matching PostgreSQL. +/// 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, - num_rows: usize, ) -> Result { /// Nanoseconds in a 24-hour day. - const DAY_NANOS: i128 = 86_400_000_000_000; + 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) + } - let left = lhs.to_array(num_rows)?; - let right = rhs.to_array(num_rows)?; + /// 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() + ) + } + } + } - // 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) + /// 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 { - (&right, &left) + (lhs, rhs) }; - 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))?; - let time_ns = time_ns_arr.as_primitive::(); - let interval_arr = cast( - interval_array, - &DataType::Interval(IntervalUnit::MonthDayNano), - )?; - let interval = interval_arr.as_primitive::(); - - let wrapped: Time64NanosecondArray = - arrow::compute::binary(time_ns, interval, |t, iv| { - let delta = iv.nanoseconds as i128; - 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)) + 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 { @@ -442,12 +484,12 @@ impl PhysicalExpr for BinaryExpr { Operator::Plus if is_time_plus_interval(&left_data_type, &right_data_type) => { - return apply_time_interval(&lhs, &rhs, false, batch.num_rows()); + 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, batch.num_rows()); + 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), diff --git a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt index 0345acc3e37a9..ee3cc02d67222 100644 --- a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt +++ b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt @@ -31,6 +31,12 @@ SELECT arrow_cast('22:00', 'Time32(Second)') + interval '3 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 D SELECT arrow_cast('22:00', 'Time32(Millisecond)') + interval '3 hours' ---- @@ -73,3 +79,24 @@ query D SELECT '02:00'::time - interval '3 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. From 9655d615b92d5952a8a245154d2b4d31a5718656 Mon Sep 17 00:00:00 2001 From: vismaytiwari Date: Thu, 16 Jul 2026 21:14:39 +0530 Subject: [PATCH 3/4] =?UTF-8?q?Address=20review:=20preserve=20the=20input?= =?UTF-8?q?=20time=20unit=20for=20`time=20=C2=B1=20interval`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rather than widening the result to `Time64(Nanosecond)`, keep the time operand's own unit and apply the interval at that resolution -- mirroring how `timestamp/date + interval` already behave via arrow's kernels (which preserve the unit and truncate finer interval precision). So `time(s) + interval '1 nanosecond'` is a no-op, exactly like `timestamp(s) + interval '1 nanosecond'`. The coercion arm now keeps the time type as the result type instead of forcing `Time64(Nanosecond)`, and `apply_time_interval` dispatches per unit through a generic `wrap_time_interval`. Updates the sqllogictests (per-unit arrow_typeof assertions + finer-than-unit truncation cases) and the 55.0.0 upgrade note. --- .../expr-common/src/type_coercion/binary.rs | 20 ++- .../physical-expr/src/expressions/binary.rs | 158 ++++++++++++------ .../datetime/arith_time_interval.slt | 38 ++++- .../library-user-guide/upgrading/55.0.0.md | 7 +- 4 files changed, 160 insertions(+), 63 deletions(-) diff --git a/datafusion/expr-common/src/type_coercion/binary.rs b/datafusion/expr-common/src/type_coercion/binary.rs index 349e0ae93badf..790c3b1e0f220 100644 --- a/datafusion/expr-common/src/type_coercion/binary.rs +++ b/datafusion/expr-common/src/type_coercion/binary.rs @@ -270,17 +270,25 @@ impl<'a> BinaryTypeCoercer<'a> { 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)`. + // is `01:30:00`). The interval is normalized to `MonthDayNano`; the time + // operand keeps its own unit and is also the result type -- mirroring + // `timestamp/date + interval`, which preserve their unit and apply the + // interval at that resolution. So, like `timestamp(s) + interval + // '1 nanosecond'`, `time(s) + interval '1 nanosecond'` is a no-op rather + // than widening the type. + let time = if let Interval(_) = lhs { + rhs.clone() + } else { + lhs.clone() + }; let (lhs, rhs) = match (lhs, rhs) { - (Interval(_), _) => (Interval(MonthDayNano), Time64(Nanosecond)), - (_, _) => (Time64(Nanosecond), Interval(MonthDayNano)), + (Interval(_), _) => (Interval(MonthDayNano), time.clone()), + (_, _) => (time.clone(), Interval(MonthDayNano)), }; return Ok(Signature { lhs, rhs, - ret: Time64(Nanosecond), + ret: time, }); } Plus | Minus | Multiply | Divide | Modulo => { diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index 078f939eecde9..94953354283c3 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -299,100 +299,154 @@ fn is_time_minus_interval(lhs: &DataType, rhs: &DataType) -> bool { ) } -/// 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. +/// 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. /// -/// 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. +/// The result keeps the input time's unit; the interval (normalized to `MonthDayNano` +/// by the coercion layer) is applied at that resolution, mirroring `timestamp + interval`. +/// Only the sub-day portion of the interval affects a time-of-day -- whole months and +/// days are ignored, matching PostgreSQL -- and any interval precision finer than the +/// time's unit is truncated (so `time(s) + interval '1 nanosecond'` is a no-op). fn apply_time_interval( lhs: &ColumnarValue, rhs: &ColumnarValue, subtract: bool, ) -> Result { + // The `time` operand determines the result type; the other is the interval. + let (time, interval) = if matches!(lhs.data_type(), DataType::Interval(_)) { + (rhs, lhs) + } else { + (lhs, rhs) + }; + + // Dispatch on the time unit; `ns_per_unit` converts the interval's nanoseconds to + // that unit, and the arithmetic is done (and wrapped) at that resolution. + match time.data_type() { + DataType::Time32(TimeUnit::Second) => wrap_time_interval::( + time, + interval, + subtract, + 1_000_000_000, + ), + DataType::Time32(TimeUnit::Millisecond) => { + wrap_time_interval::( + time, interval, subtract, 1_000_000, + ) + } + DataType::Time64(TimeUnit::Microsecond) => { + wrap_time_interval::(time, interval, subtract, 1_000) + } + DataType::Time64(TimeUnit::Nanosecond) => { + wrap_time_interval::(time, interval, subtract, 1) + } + other => internal_err!("time operand expected, got: {other}"), + } +} + +/// Adds or subtracts an interval to/from a `time` of arrow primitive type `T`, wrapping +/// the result within the 24-hour clock and keeping the type `T`. `ns_per_unit` is the +/// number of nanoseconds in one unit of `T` (e.g. `1_000` for microseconds). +fn wrap_time_interval( + time: &ColumnarValue, + interval: &ColumnarValue, + subtract: bool, + ns_per_unit: i64, +) -> Result +where + T::Native: Copy + Into + TryFrom, +{ /// 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; + // Units in a 24-hour day, at `T`'s resolution. + let day_units = DAY_NANOS / ns_per_unit; + + // Wraps `time ± interval` into `[0, day_units)`. The interval's nanoseconds are + // reduced modulo a day (so the sum stays within `i64`) and converted to `T`'s unit, + // truncating any finer precision; `rem_euclid` keeps the result non-negative when + // subtracting. The wrapped value is in `[0, day_units)`, which always fits `T::Native`. + let wrap = |time_unit: i64, iv: IntervalMonthDayNano| -> T::Native { + let delta = (iv.nanoseconds % DAY_NANOS) / ns_per_unit; let delta = if subtract { -delta } else { delta }; - (time_ns + delta).rem_euclid(DAY_NANOS) - } + let wrapped = (time_unit + delta).rem_euclid(day_units); + T::Native::try_from(wrapped).unwrap_or_default() + }; - /// Extracts a `Time64(Nanosecond)` scalar as nanoseconds since midnight. - fn time_scalar_ns(scalar: &ScalarValue) -> Result> { + /// Extracts an `Interval(MonthDayNano)` scalar. + fn interval_scalar(scalar: &ScalarValue) -> Result> { match scalar { - ScalarValue::Time64Nanosecond(value) => Ok(*value), - other => { - internal_err!( - "Time64(Nanosecond) scalar expected, got: {}", - other.data_type() - ) - } + ScalarValue::IntervalMonthDayNano(value) => Ok(*value), + other => internal_err!( + "Interval(MonthDayNano) scalar expected, got: {}", + other.data_type() + ), } } - /// Extracts an `Interval(MonthDayNano)` scalar. - fn interval_scalar(scalar: &ScalarValue) -> Result> { + /// Extracts a time scalar as its unit count since midnight. + fn time_scalar_units(scalar: &ScalarValue) -> Result> { match scalar { - ScalarValue::IntervalMonthDayNano(value) => Ok(*value), + ScalarValue::Time32Second(value) | ScalarValue::Time32Millisecond(value) => { + Ok(value.map(i64::from)) + } + ScalarValue::Time64Microsecond(value) + | ScalarValue::Time64Nanosecond(value) => Ok(*value), other => { - internal_err!( - "Interval(MonthDayNano) scalar expected, got: {}", - other.data_type() - ) + internal_err!("time 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) - }; + /// Builds a time scalar of type `P` from a unit count. + fn time_scalar(value: Option) -> ScalarValue { + match P::DATA_TYPE { + DataType::Time32(TimeUnit::Second) => { + ScalarValue::Time32Second(value.map(|v| v as i32)) + } + DataType::Time32(TimeUnit::Millisecond) => { + ScalarValue::Time32Millisecond(value.map(|v| v as i32)) + } + DataType::Time64(TimeUnit::Microsecond) => { + ScalarValue::Time64Microsecond(value) + } + _ => ScalarValue::Time64Nanosecond(value), + } + } match (time, interval) { (ColumnarValue::Array(time), ColumnarValue::Array(interval)) => { - let time = time.as_primitive::(); + let time = time.as_primitive::(); let interval = interval.as_primitive::(); - let result: Time64NanosecondArray = - arrow::compute::binary(time, interval, |t, iv| wrap(t, iv, subtract))?; + let result: PrimitiveArray = + arrow::compute::binary(time, interval, |t, iv| wrap(t.into(), iv))?; Ok(ColumnarValue::Array(Arc::new(result))) } (ColumnarValue::Array(time), ColumnarValue::Scalar(interval)) => { - let time = time.as_primitive::(); + let time = time.as_primitive::(); match interval_scalar(interval)? { Some(iv) => { - let result: Time64NanosecondArray = - time.unary(|t| wrap(t, iv, subtract)); + let result: PrimitiveArray = time.unary(|t| wrap(t.into(), iv)); Ok(ColumnarValue::Array(Arc::new(result))) } - None => Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(None))), + None => Ok(ColumnarValue::Scalar(time_scalar::(None))), } } (ColumnarValue::Scalar(time), ColumnarValue::Array(interval)) => { let interval = interval.as_primitive::(); - match time_scalar_ns(time)? { + match time_scalar_units(time)? { Some(t) => { - let result: Time64NanosecondArray = - interval.unary(|iv| wrap(t, iv, subtract)); + let result: PrimitiveArray = interval.unary(|iv| wrap(t, iv)); Ok(ColumnarValue::Array(Arc::new(result))) } - None => Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(None))), + None => Ok(ColumnarValue::Scalar(time_scalar::(None))), } } (ColumnarValue::Scalar(time), ColumnarValue::Scalar(interval)) => { - let result = time_scalar_ns(time)? + let result = time_scalar_units(time)? .zip(interval_scalar(interval)?) - .map(|(t, iv)| wrap(t, iv, subtract)); - Ok(ColumnarValue::Scalar(ScalarValue::Time64Nanosecond(result))) + .map(|(t, iv)| wrap(t, iv).into()); + Ok(ColumnarValue::Scalar(time_scalar::(result))) } } } diff --git a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt index ee3cc02d67222..57a7f78afe370 100644 --- a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt +++ b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt @@ -26,32 +26,66 @@ SELECT interval '3 hours' + '22:00'::time ---- 01:00:00 +# The result keeps the input time's unit, mirroring `timestamp + interval`, rather +# than widening to Time64(ns). query D SELECT arrow_cast('22:00', 'Time32(Second)') + interval '3 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) +Time32(s) query D SELECT arrow_cast('22:00', 'Time32(Millisecond)') + interval '3 hours' ---- 01:00:00 +query T +SELECT arrow_typeof(arrow_cast('22:00', 'Time32(Millisecond)') + interval '3 hours') +---- +Time32(ms) + query D SELECT arrow_cast('22:00', 'Time64(Microsecond)') + interval '3 hours' ---- 01:00:00 +query T +SELECT arrow_typeof(arrow_cast('22:00', 'Time64(Microsecond)') + interval '3 hours') +---- +Time64(µs) + query D SELECT arrow_cast('22:00', 'Time64(Nanosecond)') + interval '3 hours' ---- 01:00:00 +query T +SELECT arrow_typeof(arrow_cast('22:00', 'Time64(Nanosecond)') + interval '3 hours') +---- +Time64(ns) + +# Interval precision finer than the time's unit is truncated, exactly as for +# `timestamp(unit) + interval`: adding one nanosecond to a second-resolution time is a +# no-op, while a finer-resolution time keeps it. +query D +SELECT arrow_cast('22:00', 'Time32(Second)') + interval '1 nanosecond' +---- +22:00:00 + +query D +SELECT arrow_cast('12:00:00', 'Time32(Millisecond)') + interval '1 microsecond' +---- +12:00:00 + +query D +SELECT arrow_cast('12:00:00', 'Time64(Microsecond)') + interval '1 microsecond' +---- +12:00:00.000001 + # 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' 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 68ab945a18b0b..0095e23261c13 100644 --- a/docs/source/library-user-guide/upgrading/55.0.0.md +++ b/docs/source/library-user-guide/upgrading/55.0.0.md @@ -468,13 +468,14 @@ 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) +-- 55.0.0 onwards: returns a time 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)`. +months are ignored, as in PostgreSQL. The result keeps the input time's unit +(mirroring `timestamp + interval`), and any interval precision finer than that +unit is truncated -- so `time(s) + interval '1 nanosecond'` is a no-op. See [PR #23279](https://github.com/apache/datafusion/pull/23279) for details. From a968da413909817142d054031cc401f87c62c98e Mon Sep 17 00:00:00 2001 From: vismaytiwari Date: Fri, 17 Jul 2026 09:50:09 +0530 Subject: [PATCH 4/4] =?UTF-8?q?Address=20review:=20floor=20time=20=C2=B1?= =?UTF-8?q?=20interval=20like=20timestamp,=20simplify=20coercion?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Apply the interval at nanosecond precision and floor to the time's unit via div_euclid (with the sign applied before the floor), so `time(s) - interval '1 nanosecond'` rolls back a full second exactly as `timestamp(s) - interval '1 nanosecond'` does, instead of truncating toward zero. Adds an slt case for it. Also take the suggestion to bind the time type and the result type in a single match arm in the coercion. --- .../expr-common/src/type_coercion/binary.rs | 19 +++++--------- .../physical-expr/src/expressions/binary.rs | 26 ++++++++++++------- .../datetime/arith_time_interval.slt | 13 +++++++--- 3 files changed, 32 insertions(+), 26 deletions(-) diff --git a/datafusion/expr-common/src/type_coercion/binary.rs b/datafusion/expr-common/src/type_coercion/binary.rs index 54e39d7611a45..77ef1f59f7bb8 100644 --- a/datafusion/expr-common/src/type_coercion/binary.rs +++ b/datafusion/expr-common/src/type_coercion/binary.rs @@ -276,20 +276,13 @@ impl<'a> BinaryTypeCoercer<'a> { // interval at that resolution. So, like `timestamp(s) + interval // '1 nanosecond'`, `time(s) + interval '1 nanosecond'` is a no-op rather // than widening the type. - let time = if let Interval(_) = lhs { - rhs.clone() - } else { - lhs.clone() - }; - let (lhs, rhs) = match (lhs, rhs) { - (Interval(_), _) => (Interval(MonthDayNano), time.clone()), - (_, _) => (time.clone(), Interval(MonthDayNano)), + let (lhs, rhs, ret) = match (lhs, rhs) { + (Interval(_), time) => { + (Interval(MonthDayNano), time.clone(), time.clone()) + } + (time, _) => (time.clone(), Interval(MonthDayNano), time.clone()), }; - return Ok(Signature { - lhs, - rhs, - ret: time, - }); + return Ok(Signature { lhs, rhs, ret }); } Plus | Minus | Multiply | Divide | Modulo => { if let Ok(ret) = self.get_result(lhs, rhs) { diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index 30fcf30d717c0..e182bc14fa833 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -302,10 +302,12 @@ fn is_time_minus_interval(lhs: &DataType, rhs: &DataType) -> bool { /// not implement time-of-day arithmetic, so it is handled here. /// /// The result keeps the input time's unit; the interval (normalized to `MonthDayNano` -/// by the coercion layer) is applied at that resolution, mirroring `timestamp + interval`. -/// Only the sub-day portion of the interval affects a time-of-day -- whole months and -/// days are ignored, matching PostgreSQL -- and any interval precision finer than the -/// time's unit is truncated (so `time(s) + interval '1 nanosecond'` is a no-op). +/// by the coercion layer) is applied at nanosecond precision and floored to that unit, +/// mirroring `timestamp(unit) + interval`. Only the sub-day portion of the interval +/// affects a time-of-day -- whole months and days are ignored, matching PostgreSQL. The +/// floor is applied after the sign, so `time(s) + interval '1 nanosecond'` is a no-op +/// while `time(s) - interval '1 nanosecond'` rolls back a second, exactly as the +/// timestamp case does. fn apply_time_interval( lhs: &ColumnarValue, rhs: &ColumnarValue, @@ -359,13 +361,17 @@ where // Units in a 24-hour day, at `T`'s resolution. let day_units = DAY_NANOS / ns_per_unit; - // Wraps `time ± interval` into `[0, day_units)`. The interval's nanoseconds are - // reduced modulo a day (so the sum stays within `i64`) and converted to `T`'s unit, - // truncating any finer precision; `rem_euclid` keeps the result non-negative when - // subtracting. The wrapped value is in `[0, day_units)`, which always fits `T::Native`. + // Wraps `time ± interval` into `[0, day_units)`. The interval is reduced modulo a day + // (so the sum stays within `i64`), applied at nanosecond precision, then floored to + // `T`'s unit -- matching `timestamp(unit) ± interval`. Because the floor is applied + // after the sign, `time(s) - interval '1 nanosecond'` rolls back a full second, just + // as the timestamp case does, while `time(s) + interval '1 nanosecond'` is a no-op. + // `div_euclid`/`rem_euclid` floor toward negative infinity, so the wrapped value stays + // in `[0, day_units)`, which always fits `T::Native`. let wrap = |time_unit: i64, iv: IntervalMonthDayNano| -> T::Native { - let delta = (iv.nanoseconds % DAY_NANOS) / ns_per_unit; - let delta = if subtract { -delta } else { delta }; + let iv_ns = iv.nanoseconds % DAY_NANOS; + let signed_ns = if subtract { -iv_ns } else { iv_ns }; + let delta = signed_ns.div_euclid(ns_per_unit); let wrapped = (time_unit + delta).rem_euclid(day_units); T::Native::try_from(wrapped).unwrap_or_default() }; diff --git a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt index 57a7f78afe370..1d2b0e15bb953 100644 --- a/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt +++ b/datafusion/sqllogictest/test_files/datetime/arith_time_interval.slt @@ -68,14 +68,21 @@ SELECT arrow_typeof(arrow_cast('22:00', 'Time64(Nanosecond)') + interval '3 hour ---- Time64(ns) -# Interval precision finer than the time's unit is truncated, exactly as for -# `timestamp(unit) + interval`: adding one nanosecond to a second-resolution time is a -# no-op, while a finer-resolution time keeps it. +# The interval is applied at nanosecond precision and floored to the time's unit, exactly +# as for `timestamp(unit) ± interval`. Adding one nanosecond to a second-resolution time +# floors back to a no-op... query D SELECT arrow_cast('22:00', 'Time32(Second)') + interval '1 nanosecond' ---- 22:00:00 +# ...but subtracting one nanosecond floors down a full second, matching +# `timestamp(s) - interval '1 nanosecond'` (= 09:59:59) rather than staying put. +query D +SELECT arrow_cast('10:00:00', 'Time32(Second)') - interval '1 nanosecond' +---- +09:59:59 + query D SELECT arrow_cast('12:00:00', 'Time32(Millisecond)') + interval '1 microsecond' ----