expr: clamp round(numeric, scale) instead of overflowing - #38277
Open
def- wants to merge 1 commit into
Open
Conversation
`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 read the exponent alone, which is wrong at both ends of
its range.
It tested `a_exp > 0`, missing `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 to `a_exp >= 0` 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. Testing finiteness keeps 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) <noreply@anthropic.com>
def-
marked this pull request as ready for review
August 19, 2026 15:27
Contributor
Author
|
Ready for review now. Feel free to merge in my absence or fix it up! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
round(a, b)right-pads with zeroes viarescalewhenbreaches pasta's fractional digits, and otherwise shifts left byb, rounds, and shifts back. The branch condition read the exponent alone, which is wrong at both ends of its range.It tested
a_exp > 0, missinga_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, soround(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_rangesfound it asround(extract(epoch from date '2000-01-01'), 2147483647).Widening to
a_exp >= 0alone would swallow the special values, which report an exponent of zero as well.rescaleon an infinity is an invalid operation: it yieldsNaNand setsinvalid_operation, never theoverflowthe function checks, soround('Infinity'::numeric, 2)would answerNaN. That breaks the same interpreter the other direction, becauseNaNsorts as the maximum: a declared-monotoneroundmapping both ends of[-Infinity, Infinity]toNaNnarrows the output toNaNalone and rules out every finite value the evaluator produces in between. That is what the secondtest_equivalence_rangesfailure was, reported asrow=Numeric(0) value=Numeric(0) spec=Within(Numeric(NaN), Numeric(NaN)). Testing finiteness keeps the specials on the rounding path, which propagates them unchanged.Tests
test/sqllogictest/numeric.sltgains the exponent-zero case (round(extract(epoch from date '2000-01-01'), 2147483647)), the clamped-scale cases (round(123::numeric, 38),round(5::numeric, 2147483647)), and the infinity case.test/testdrive/decimal-overflow.tdswaps theROUNDoverflow assertion to a negative scale, which still overflows, and asserts that a positive scale no longer does.test_equivalence_rangesis a proptest seeded from entropy, so it only failed intermittently in CI. Locally the second failure reproduced in 15 of 20 runs before the fix and in 0 of 70 after it.Release notes
This release will fix
round(<numeric>, <scale>)erroring on a scale that only right-pads a value with zeroes, and no longer turnInfinityintoNaNwhen a scale is given.Closes: CPU-206