peterxcli commented on code in PR #10409:
URL: https://github.com/apache/arrow-rs/pull/10409#discussion_r3657956393
##########
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:
Thanks. I agree with caller-side coercion for the numeric operand: callers
can coerce multiplication and division factors to `Int64` or `Float64`, so
`arrow-arith` does not need to support every numeric type.
**I’m less clear that the same precedent requires coercing the interval
representation. [#10336](https://github.com/apache/arrow-rs/pull/10336)
directly supports `Interval(YearMonth)`, `Interval(DayTime)`, and
`Interval(MonthDayNano)` for multiplication by `Int64`; this PR follows the
same input coverage. For `Float64`, the result must be `MonthDayNano` because
fractional months and days can cascade into smaller components, and widening
`YearMonth`/`DayTime` to `MonthDayNano` is lossless.**
If callers should also normalize the interval representation, should the
same rule apply to #10336, or is the distinction that integer multiplication
preserves the input representation while floating-point arithmetic widens it?
Could you also point me to a specific kernel or PR establishing this
precedent for coercion between related Arrow types? The precedent discussed in
[#6906](https://github.com/apache/arrow-rs/pull/6906#discussion_r1894461220)
appears to concern heterogeneous numeric operands; limiting factors to `Int64`
or `Float64` already follows that boundary.
--
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]
