vismaytiwari commented on code in PR #23279:
URL: https://github.com/apache/datafusion/pull/23279#discussion_r3526517423
##########
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<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))?;
+ let time_ns = time_ns_arr.as_primitive::<Time64NanosecondType>();
+ let interval_arr = cast(
+ interval_array,
+ &DataType::Interval(IntervalUnit::MonthDayNano),
Review Comment:
Good catch — dropped this one. The coercion already normalizes the interval
to MonthDayNano, so we can downcast directly here.
##########
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<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)?;
+
Review 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.
##########
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<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))?;
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
