Apply only the caller's rounding mode in round/3 - #248
Merged
Conversation
tomciopp
force-pushed
the
round-mode-fix
branch
from
August 27, 2026 18:44
81738f1 to
bf89844
Compare
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.
Fixes #247 . Implements the minimal fix agreed there: round to
placesundermode, with only the result going through the context; no new signalling; andthe exponent limits kept on the input.
The bug
round/3rounded twice for any value whose coefficient carried moresignificant digits than the context precision. The first rounding used
Context.get().rounding, not themodeargument, so the result could violatethe defining property of the mode the caller asked for.
The context's
:half_upcarried0.4995to0.500;:downthen truncatedthat. Truncating the input gives
0.4.The context's mode intrudes in both directions, truncating first under
:floordiscards the nonzero digits:ceilingneeds to see:Cause
round/3normalized its input, andnormalize/1applies the context on bothof its non-zero branches.
context/5rounds the coefficient toctx.precisionunder
ctx.rounding, sodo_round/5received a value already rounded under amode the caller never asked for.
Reachability
At the default context the parse limit (
max_digits: 34) and the precision(34) coincide, so an over-precision literal never survives
Decimal.new/1it is rejected as a parse error. That check guards only the parsing path though,
and
new/3performs no digit count.new/3is the path a database driver takes.Postgrex.Extensions.Numericdecodes a
numericby accumulating base-10000 digit groups from the binarywire format and calling
Decimal.new(sign, coef, -scale), so nothing consultsmax_digits. A Postgresnumericwith no type modifier is unconstrained, so acolumn value or an
AVG(numeric)result can carry far more than 34significant digits straight into
round/3at the default context:That is the construction Postgrex produces, and the regression test uses it.
The fix
Drop the precision pass on the input. This is the bug itself: the rounding
that used
ctx.roundingin place ofmode.Keep the exponent limits on the input, via a new
exponent_limited/2— theexponent half of
context/5without the precision half.do_round/5scalesthe coefficient by
pow10(exp - target_exp)whenexp > target_exp, which isunbounded for an exponent built through
new/3; without the check,Decimal.round(Decimal.new(1, 1, 10_000_000), 0)would build aten-million-digit coefficient only for the context to discard it. Checking
before the zero-strip rather than after is safe: stripping moves
expand thedigit count in opposite directions by the same amount, so the adjusted exponent
is identical either way.
Because that check can itself overflow to Infinity,
round/3keeps a%Decimal{coef: :inf}case arm. It catches an overflow produced there, not anInfinity passed in, the function clause above already handles that.
Strip trailing zeros directly with
strip_trailing_zeros/2, which is whatnormalize/1contributed besides the context call. This one is an optimizationrather than a correctness requirement.
split_digits/3handles trailing zerosidentically with or without it, but it keeps
do_round/5off needlessly widecoefficients.
Those three are also sufficient: nothing else between the input and
do_round/5rounds, somodeis the only roundinground/3now applies. Thefinal
context/4call on the result is unchanged and still bounds it to theprecision. That is the same contract every other operation has, and it acts on
the rounded result rather than on the digits
modewas deciding about.Behavior changes
Inputs wider than the precision round once, under the caller's mode.
round/3no longer sets:inexact/:roundedfor an over-precision input.It never signalled for the digits it discards itself, so it now signals for
neither rather than only in that incidental case, keeping it a
round-to-integral-value style operation that does not signal for discarded
digits.
Not a regression
Long-standing, not introduced by #240 or #241. At
eb75d13, the base commitfor both,
round/3already callednormalize/1andnormalize/1alreadyended in
|> context.Tests
:down,:ceilingand:floorcontextsnew/3case at the default precision10^7exponent still overflows withoutmaterializing the coefficient
Existing
round/3tests pass unchanged: they use small values at the defaultprecision, where the removed pass was already a no-op.