Jefffrey commented on code in PR #23279:
URL: https://github.com/apache/datafusion/pull/23279#discussion_r3524546295
##########
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:
we have some optimization opportunity here to operate directly on
columnarvalue instead of converting to arrays; see `apply_date_subtraction` for
reference
##########
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:
this cast to monthdaynano should be unnecessary as logical layer coerces to
the right type
##########
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),
+ )?;
+ let interval = interval_arr.as_primitive::<IntervalMonthDayNanoType>();
+
+ let wrapped: Time64NanosecondArray =
+ arrow::compute::binary(time_ns, interval, |t, iv| {
+ let delta = iv.nanoseconds as i128;
Review Comment:
why is operating on `i128` necessary 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)?;
+
+ // 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:
similarly here i feel we can avoid this cast
--
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]
