peterxcli commented on code in PR #10409:
URL: https://github.com/apache/arrow-rs/pull/10409#discussion_r3662844791
##########
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:
> since i64 multiplication preserves input type, its not doing any coercion
so its fine as is. f64 requires converting to monthdaynano so this is something
the caller should cast to instead of us casting inside an arithmetic kernel
> just in general, this type of input casting is best left to the caller to
opt in to, and at the arrow-rs level its simpler to just support the exact
types we actually support (in this cast f64 * monthdaynano)
sounds fair, let me remove the conversion and only keep the
`Interval(MonthDayNano) * f64` input. Thanks for the explaination!
--
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]
