Jefffrey commented on code in PR #10409:
URL: https://github.com/apache/arrow-rs/pull/10409#discussion_r3679836691
##########
arrow-arith/src/numeric.rs:
##########
@@ -724,6 +729,123 @@ fn interval_mul_op<T: IntervalOp>(
))
}
+/// Multiplies using `IntervalMonthDayNano` as the common interval
representation,
Review Comment:
this docstring is out of date; no need to mention "common interval
representation" since its only defined for monthdaynano intervals
##########
arrow-arith/src/numeric.rs:
##########
@@ -724,6 +729,123 @@ 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>
+///
+/// Algorithm:
+///
+/// 1. use checked integer multiplication when `factor` fits in `i64` (early
return).
+/// 2. multiply months and days separately and truncate floating point.
+/// 3. cascade remainders: convert fractional months to days using 30
+/// days per month, then fractional days to a sub-day value using 24 hours
+/// per day.
+/// 4. combine the cascaded remainder with the scaled input nanoseconds and
round ties-to-even at nanosecond precision.
+/// 5. return an overflow error if any output component is out of
+/// range.
+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 is referenced
from PostgreSQL's 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"))?;
Review Comment:
does `to_i32()` not check the range and non-finite numbers when converting?
trying to understand why we have two checks here
##########
arrow-arith/src/numeric.rs:
##########
@@ -724,6 +729,123 @@ 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>
+///
+/// Algorithm:
+///
+/// 1. use checked integer multiplication when `factor` fits in `i64` (early
return).
+/// 2. multiply months and days separately and truncate floating point.
+/// 3. cascade remainders: convert fractional months to days using 30
+/// days per month, then fractional days to a sub-day value using 24 hours
+/// per day.
+/// 4. combine the cascaded remainder with the scaled input nanoseconds and
round ties-to-even at nanosecond precision.
+/// 5. return an overflow error if any output component is out of
+/// range.
+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 is referenced
from PostgreSQL's 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(
+ op: Op,
+ interval: &dyn Array,
+ interval_scalar: bool,
+ factor: &dyn Array,
+ factor_scalar: bool,
+) -> Result<ArrayRef, ArrowError> {
+ let interval = interval.as_primitive::<IntervalMonthDayNanoType>();
+ let factor = factor.as_primitive::<Float64Type>();
+ Ok(try_op_ref!(
+ IntervalMonthDayNanoType,
+ interval,
+ interval_scalar,
+ factor,
+ factor_scalar,
+ {
+ match op {
+ Op::Mul => interval_mul_f64(interval, factor),
+ Op::Div if factor == 0. => Err(ArrowError::DivideByZero),
+ // DuckDB defines interval division as multiplication by the
reciprocal:
+ //
https://github.com/duckdb/duckdb/blob/21aca0424f1faf78b593b1e6fbfdd4846624c987/src/function/scalar/operator/arithmetic.cpp#L1102-L1110
+ Op::Div => interval_mul_f64(interval, 1. / factor),
+ _ => unreachable!(),
Review Comment:
this `unreachable!()` seems reachable in normal usage (if we use `add(mdn,
f64)` for example)
##########
arrow-arith/src/numeric.rs:
##########
@@ -724,6 +729,123 @@ 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>
+///
+/// Algorithm:
+///
+/// 1. use checked integer multiplication when `factor` fits in `i64` (early
return).
+/// 2. multiply months and days separately and truncate floating point.
+/// 3. cascade remainders: convert fractional months to days using 30
+/// days per month, then fractional days to a sub-day value using 24 hours
+/// per day.
+/// 4. combine the cascaded remainder with the scaled input nanoseconds and
round ties-to-even at nanosecond precision.
+/// 5. return an overflow error if any output component is out of
+/// range.
+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 is referenced
from PostgreSQL's 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);
Review Comment:
could we use f64::fract()` here instead?
--
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]
