From b5fa09d2aa05a025dd391fe0194ce2718a1b679f Mon Sep 17 00:00:00 2001 From: Dennis Felsing Date: Mon, 17 Aug 2026 23:52:02 +0200 Subject: [PATCH] expr: clamp round(numeric, scale) instead of overflowing `round(a, b)` right-pads with zeroes via `rescale` when `b` reaches past `a`'s fractional digits, and otherwise shifts left by `b`, rounds, and shifts back. The branch condition tested `a_exp > 0`, reading the exponent alone, which is wrong at both ends of its range. It missed `a_exp == 0`, the representation of a value with neither fractional digits nor trailing zeroes. Those values took the rounding path, which shifts left by the scale first and overflows the exponent range, so `round(123::numeric, 38)` errored where PostgreSQL returns a value. Row encoding folds trailing zeroes into the exponent, so the result depended on which representation of an equal value reached the function. That let the abstract interpreter, which reads its datums back out of a row, call an expression infallible that the evaluator failed on, so persist filter pushdown could discard a part it has to keep. `test_equivalence_ranges` found it as `round(extract(epoch from date '2000-01-01'), 2147483647)`. Widening the exponent test alone would swallow the special values, which report an exponent of zero as well. `rescale` on an infinity is an invalid operation: it yields `NaN` and sets `invalid_operation`, never the `overflow` the function checks, so `round('Infinity'::numeric, 2)` would answer `NaN`. That breaks the same interpreter the other direction, because `NaN` sorts as the maximum: a declared-monotone `round` mapping both ends of `[-Infinity, Infinity]` to `NaN` narrows the output to `NaN` alone and rules out every finite value the evaluator produces in between. The branch now rescales exactly when `a` is finite and `b` reaches past its scale, keeping the specials on the rounding path, which propagates them unchanged. Extends `test/sqllogictest/numeric.slt` with the exponent-zero, clamped-scale, and infinity cases, and `test/testdrive/decimal-overflow.td` with the positive scale that no longer overflows. Closes: CPU-206 Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Fable 5 --- src/expr/src/interpret.rs | 42 ++++++++++++++++++++++++++++++ src/expr/src/scalar/func.rs | 25 +++++++++++++----- test/sqllogictest/numeric.slt | 25 ++++++++++++++++++ test/testdrive/decimal-overflow.td | 9 +++++-- 4 files changed, 92 insertions(+), 9 deletions(-) diff --git a/src/expr/src/interpret.rs b/src/expr/src/interpret.rs index 0ea5390de5413..5bba046327695 100644 --- a/src/expr/src/interpret.rs +++ b/src/expr/src/interpret.rs @@ -3360,6 +3360,48 @@ mod tests { ); } + /// `round` must not depend on a numeric's exponent: `Row` encoding folds + /// trailing zeroes into it, so the interpreter, which reads its datums back + /// out of a `Row`, would otherwise disagree with the evaluator and pushdown + /// could discard a part it has to keep. + #[mz_ore::test] + #[cfg_attr(miri, ignore)] + fn test_round_numeric_representation_independent() { + use mz_repr::adt::date::Date; + + let arena = RowArena::new(); + let lit = |d: Datum, ty: ReprScalarType| { + let mut row = Row::default(); + row.packer().push(d); + MirScalarExpr::Literal(Ok(row), ty.nullable(false)) + }; + + // `extract` hands `round` a `946684800` whose exponent is zero, where + // the same value read out of a `Row` is `9.466848E+8`. + let expr = lit( + Datum::Date(Date::from_pg_epoch(0).unwrap()), + ReprScalarType::Date, + ) + .call_unary(UnaryFunc::ExtractDate(ExtractDate(DateTimeUnits::Epoch))) + .call_binary( + lit(Datum::Int32(i32::MAX), ReprScalarType::Int32), + BinaryFunc::from(RoundNumericBinary), + ); + + let relation = ReprRelationType::new(vec![]); + let range = ColumnSpecs::new(&relation, &arena).expr(&expr).range; + match expr.eval(&[], &arena) { + Ok(value) => assert!( + range.may_contain(value), + "interpreter ruled out {value:?}, which the evaluator produced: {range:?}", + ), + Err(_) => assert!( + range.may_fail(), + "interpreter ruled out the error the evaluator produced: {range:?}", + ), + } + } + #[mz_ore::test] fn test_trace() { use super::Trace; diff --git a/src/expr/src/scalar/func.rs b/src/expr/src/scalar/func.rs index 9dd82c1392921..40b55d2c4b75d 100644 --- a/src/expr/src/scalar/func.rs +++ b/src/expr/src/scalar/func.rs @@ -480,17 +480,28 @@ fn add_time_interval(time: chrono::NaiveTime, interval: Interval) -> chrono::Nai fn round_numeric_binary(a: OrderedDecimal, mut b: i32) -> Result { let mut a = a.0; let mut cx = numeric::cx_datum(); - let a_exp = a.exponent(); - if a_exp > 0 && b > 0 || a_exp < 0 && -a_exp < b { - // This condition indicates: - // - a is a value without a decimal point, b is a positive number - // - a has a decimal point, but b is larger than its scale - // In both of these situations, right-pad the number with zeroes, which // is most easily done with rescale. + let a_scale = numeric::get_scale(&a); + if a.is_finite() && i64::from(b) > i64::from(a_scale) { + // Rounding at or past `a`'s scale cannot change the value: right-pad + // with zeroes via rescale. The rounding path below shifts left by `b` + // first and overflows for large `b`. + // + // NOTE: Equal values can reach here with different scales, since + // `Row` encoding folds trailing zeroes into the exponent. The result, + // value or error, must not depend on which representation arrives: the + // abstract interpreter reads its datums back out of a `Row`, and if it + // calls infallible what the evaluator fails on, persist filter + // pushdown discards parts it has to keep. + // + // `Infinity` and `NaN` report a scale of zero, but `rescale` on an + // infinity yields `NaN` via invalid_operation, not the overflow + // checked below, so the specials take the rounding path, which + // propagates them unchanged, as PostgreSQL does. // Ensure rescale doesn't exceed max precision by putting a ceiling on // b equal to the maximum remaining scale the value can support. let max_remaining_scale = u32::from(numeric::NUMERIC_DATUM_MAX_PRECISION) - - (numeric::get_precision(&a) - numeric::get_scale(&a)); + - (numeric::get_precision(&a) - a_scale); b = match i32::try_from(max_remaining_scale) { Ok(max_remaining_scale) => std::cmp::min(b, max_remaining_scale), Err(_) => b, diff --git a/test/sqllogictest/numeric.slt b/test/sqllogictest/numeric.slt index 24be504a442d7..39701ae7d7474 100644 --- a/test/sqllogictest/numeric.slt +++ b/test/sqllogictest/numeric.slt @@ -972,6 +972,31 @@ SELECT round(6e38, 39) ---- 600000000000000000000000000000000000000 +# A scale above the value's fractional digits cannot change it, so it clamps to +# what the datum holds rather than overflowing. +query R +SELECT round(123::numeric, 38) +---- +123 + +query R +SELECT round(5::numeric, 2147483647) +---- +5 + +# `extract` hands `round` a numeric that never passed through a row, so its +# exponent is zero where an equal value read from a row carries a positive one. +query R +SELECT round(extract(epoch FROM DATE '2000-01-01'), 2147483647) +---- +946684800 + +# The infinities carry an exponent of zero too, but a scale cannot change them. +query RR +SELECT round(sum(f1), 2), round(-sum(f1), 2) FROM (VALUES ('999999999999999999999999999999999999999'::numeric), ('999999999999999999999999999999999999999')) t (f1) +---- +Infinity -Infinity + query R SELECT round(19.87, -1) ---- diff --git a/test/testdrive/decimal-overflow.td b/test/testdrive/decimal-overflow.td index d7d2b75419893..af6c03851c440 100644 --- a/test/testdrive/decimal-overflow.td +++ b/test/testdrive/decimal-overflow.td @@ -32,10 +32,15 @@ contains:value out of range: underflow ! SELECT '999999999999999999999999999999999999999'::decimal * 10::decimal; contains:value out of range: overflow -# ROUND creates a value that is too large -! SELECT ROUND('999999999999999999999999999999999999999'::decimal,1); +# ROUND carries into a value that is too large +! SELECT ROUND('999999999999999999999999999999999999999'::decimal,-1); contains:value out of range: overflow +# A positive scale only right-pads with zeroes, so it cannot make the value too +# large. The scale clamps to what the datum can hold. +> SELECT ROUND('999999999999999999999999999999999999999'::decimal,1); +999999999999999999999999999999999999999 + # POW ! SELECT POW(99999::decimal,9); contains:value out of range: overflow