Jefffrey commented on code in PR #10409:
URL: https://github.com/apache/arrow-rs/pull/10409#discussion_r3653746334
##########
arrow-arith/src/numeric.rs:
##########
@@ -724,6 +745,116 @@ fn interval_mul_op<T: IntervalOp>(
))
}
+/// Multiplies using `IntervalMonthDayNano` as the common interval
representation,
+/// mirroring DuckDB's `interval_t` layout of months, days, and a sub-day
component
+/// (nanoseconds in Arrow, microseconds in DuckDB).
+///
+///
<https://github.com/duckdb/duckdb/blob/21aca0424f1faf78b593b1e6fbfdd4846624c987/src/include/duckdb/common/types/interval.hpp#L24-L27>
+fn interval_mul_f64(
+ interval: IntervalMonthDayNano,
+ factor: f64,
+) -> Result<IntervalMonthDayNano, ArrowError> {
+ const DAYS_PER_MONTH: f64 = 30.;
+ const NANOS_PER_SECOND: f64 = NANOSECONDS as f64;
+ const SECONDS_PER_DAY: f64 = SECONDS_IN_DAY as f64;
+
+ // Keep integral factors exact instead of round-tripping i64 nanoseconds
through f64.
+ if factor.fract() == 0. {
+ if let Some(factor) = ToPrimitive::to_i64(&factor) {
+ return IntervalMonthDayNanoType::mul_i64(interval, factor);
+ }
+ }
+
+ // Based on DuckDB's INTERVAL * DOUBLE implementation, which it says was
+ // "Taken from Postgres src.backend/utils/adt/timestamp.c:interval_mul":
+ //
https://github.com/duckdb/duckdb/blob/21aca0424f1faf78b593b1e6fbfdd4846624c987/src/function/scalar/operator/multiply.cpp#L48-L123
+ // PostgreSQL's interval_mul:
+ //
https://github.com/postgres/postgres/blob/78758d37306cd89ab060f00cb06f249018d5b8da/src/backend/utils/adt/timestamp.c#L3627-L3744
+ let overflow =
+ |component| ArrowError::ArithmeticOverflow(format!("Overflow in
interval {component}"));
+ let timestamp_round =
+ |value: f64| (value * NANOS_PER_SECOND).round_ties_even() /
NANOS_PER_SECOND;
+
+ let months_product = f64::from(interval.months) * factor;
+ if !months_product.is_finite()
+ || months_product < f64::from(i32::MIN)
+ || months_product > f64::from(i32::MAX)
+ {
+ return Err(overflow("months"));
+ }
+ let months = months_product.to_i32().ok_or_else(|| overflow("months"))?;
+
+ let days_product = f64::from(interval.days) * factor;
+ if !days_product.is_finite()
+ || days_product < f64::from(i32::MIN)
+ || days_product > f64::from(i32::MAX)
+ {
+ return Err(overflow("days"));
+ }
+ let mut days = days_product.to_i32().ok_or_else(|| overflow("days"))?;
+
+ let month_remainder = timestamp_round((months_product - f64::from(months))
* DAYS_PER_MONTH);
+ let month_remainder_days = month_remainder
+ .to_i32()
+ .ok_or_else(|| overflow("month remainder"))?;
+ let mut seconds_remainder = timestamp_round(
+ (days_product - f64::from(days) + month_remainder -
f64::from(month_remainder_days))
+ * SECONDS_PER_DAY,
+ );
+
+ if seconds_remainder.abs() >= SECONDS_PER_DAY {
+ let remainder_days = (seconds_remainder / SECONDS_PER_DAY)
+ .to_i32()
+ .ok_or_else(|| overflow("day remainder"))?;
+ days = days
+ .checked_add(remainder_days)
+ .ok_or_else(|| overflow("days"))?;
+ seconds_remainder -= f64::from(remainder_days) * SECONDS_PER_DAY;
+ }
+ days = days
+ .checked_add(month_remainder_days)
+ .ok_or_else(|| overflow("days"))?;
+
+ let nanoseconds = ((interval.nanoseconds as f64) * factor
+ + seconds_remainder * NANOS_PER_SECOND)
+ .round_ties_even();
+ let nanoseconds = ToPrimitive::to_i64(&nanoseconds).ok_or_else(|| {
+ ArrowError::ArithmeticOverflow(format!("Overflow in interval
nanoseconds: {nanoseconds}"))
+ })?;
+
+ Ok(IntervalMonthDayNano::new(months, days, nanoseconds))
+}
+
+fn interval_f64_op<T: IntervalOp>(
+ op: Op,
+ interval: &dyn Array,
+ interval_scalar: bool,
+ factor: &dyn Array,
+ factor_scalar: bool,
+) -> Result<ArrayRef, ArrowError> {
+ let interval = interval.as_primitive::<T>();
+ let factor = factor.as_primitive::<Float64Type>();
+ Ok(try_op_ref!(
+ IntervalMonthDayNanoType,
+ interval,
+ interval_scalar,
+ factor,
+ factor_scalar,
+ {
+ // Float scaling can create components that narrower interval
units cannot store.
+ let interval = T::to_month_day_nano(interval);
Review Comment:
i dont think we should have this conversion logic in arrow-rs; we should
probably support only `IntervalMonthDay` explicitly and expect callers (e.g.
datafusion) to coerce to this type themselves if they want to use this kernel.
i think that has been the precedent for other kernels in arrow-rs
##########
arrow-arith/src/numeric.rs:
##########
@@ -249,6 +251,12 @@ fn arithmetic_op(op: Op, lhs: &dyn Datum, rhs: &dyn Datum)
-> Result<ArrayRef, A
(Int64, Interval(YearMonth)) if matches!(op, Op::Mul) =>
interval_mul_op::<IntervalYearMonthType>(r, r_scalar, l, l_scalar),
(Int64, Interval(DayTime)) if matches!(op, Op::Mul) =>
interval_mul_op::<IntervalDayTimeType>(r, r_scalar, l, l_scalar),
(Int64, Interval(MonthDayNano)) if matches!(op, Op::Mul) =>
interval_mul_op::<IntervalMonthDayNanoType>(r, r_scalar, l, l_scalar),
+ (Interval(YearMonth), Float64) if matches!(op, Op::Mul | Op::Div) =>
interval_f64_op::<IntervalYearMonthType>(op, l, l_scalar, r, r_scalar),
+ (Interval(DayTime), Float64) if matches!(op, Op::Mul | Op::Div) =>
interval_f64_op::<IntervalDayTimeType>(op, l, l_scalar, r, r_scalar),
+ (Interval(MonthDayNano), Float64) if matches!(op, Op::Mul | Op::Div)
=> interval_f64_op::<IntervalMonthDayNanoType>(op, l, l_scalar, r, r_scalar),
+ (Float64, Interval(YearMonth)) if matches!(op, Op::Mul) =>
interval_f64_op::<IntervalYearMonthType>(op, r, r_scalar, l, l_scalar),
+ (Float64, Interval(DayTime)) if matches!(op, Op::Mul) =>
interval_f64_op::<IntervalDayTimeType>(op, r, r_scalar, l, l_scalar),
+ (Float64, Interval(MonthDayNano)) if matches!(op, Op::Mul) =>
interval_f64_op::<IntervalMonthDayNanoType>(op, r, r_scalar, l, l_scalar),
Review Comment:
we should probably clean this up a little:
- fold all into a common `interval_op()` function like `timestamp_op()`
above, where `interval_op()` only expects the left type and will match on the
right type + op inside the function
- dont have both `interval * float` and `float * interval` here, rely on
fixing the catch all arm below for having commutative support
##########
arrow-arith/src/numeric.rs:
##########
@@ -724,6 +745,116 @@ fn interval_mul_op<T: IntervalOp>(
))
}
+/// Multiplies using `IntervalMonthDayNano` as the common interval
representation,
+/// mirroring DuckDB's `interval_t` layout of months, days, and a sub-day
component
+/// (nanoseconds in Arrow, microseconds in DuckDB).
+///
+///
<https://github.com/duckdb/duckdb/blob/21aca0424f1faf78b593b1e6fbfdd4846624c987/src/include/duckdb/common/types/interval.hpp#L24-L27>
+fn interval_mul_f64(
+ interval: IntervalMonthDayNano,
+ factor: f64,
+) -> Result<IntervalMonthDayNano, ArrowError> {
+ const DAYS_PER_MONTH: f64 = 30.;
+ const NANOS_PER_SECOND: f64 = NANOSECONDS as f64;
+ const SECONDS_PER_DAY: f64 = SECONDS_IN_DAY as f64;
+
+ // Keep integral factors exact instead of round-tripping i64 nanoseconds
through f64.
+ if factor.fract() == 0. {
+ if let Some(factor) = ToPrimitive::to_i64(&factor) {
+ return IntervalMonthDayNanoType::mul_i64(interval, factor);
+ }
+ }
+
+ // Based on DuckDB's INTERVAL * DOUBLE implementation, which it says was
+ // "Taken from Postgres src.backend/utils/adt/timestamp.c:interval_mul":
+ //
https://github.com/duckdb/duckdb/blob/21aca0424f1faf78b593b1e6fbfdd4846624c987/src/function/scalar/operator/multiply.cpp#L48-L123
+ // PostgreSQL's interval_mul:
+ //
https://github.com/postgres/postgres/blob/78758d37306cd89ab060f00cb06f249018d5b8da/src/backend/utils/adt/timestamp.c#L3627-L3744
+ let overflow =
Review Comment:
could we have an overview of the algorithm here, perhaps in the docstring?
would be good to be able to understand at a glance without looking at the code
or needing to go to duckdb/postgres code
--
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]
