Repository navigation
Conversation
Rounding an inexact quotient up can carry into an extra digit, so at precision 9, 999999999.5/1 gives 1000000000 rather than the specified 1.00000000E+9. Add a test asserting the current results.
Rounding an inexact quotient up could carry into an extra digit, so at precision 9, 999999999.5/1 gave 1000000000 rather than 1.00000000E+9. Drop the new trailing zero and raise the exponent instead, as Round does. The GDA test suite compares finite results numerically, which hid this. An inexact quotient always has Precision digits, so compare those results exactly.
Context.Quo gives every exact quotient Precision digits, so that at precision 34, 6.00/3 gives 2.000000000000000000000000000000000 rather than 2.00, the ideal exponent of the specification. Add test cases asserting the current results. Also check that Exp, which sums a series through Quo, keeps Precision digits for a small x, where the series is short and exact.
Quo computed every quotient with Precision digits, so at precision 34, 6.00/3 gave 2.000000000000000000000000000000000. The specification instead gives an exact quotient the ideal exponent, the dividend's less the divisor's, or the closest one within the precision, and only an inexact quotient keeps Precision digits: 6.00/3 is 2.00, 1/4 is 0.25, 1000000/1E+4 is 100.0000, and at precision 9, 1000000000000/1 is 1.00000000E+12 with Rounded. Add, Sub and Mul already follow their ideal exponents, and downstream users reduce exact quotients by hand today. Until v3.1.0, Quo stripped every trailing zero, going past the ideal exponent: 10.0000/4.0 gave 2.5 rather than 2.500. Pull request cockroachdb#115 removed that to keep the digits of the computation like PostgreSQL does. Reducing only to the ideal exponent is the middle ground: the zeros implied by the operands stay, an inexact quotient keeps Precision digits, and a caller wanting a fixed scale quantizes the result, as it must for inexact quotients already. After the division, the exponent of an exact quotient is a known number of places below the ideal one, so remove at most that many trailing zeros. The bounded helper is shared with Decimal.Reduce, and divides by 10^k for decreasing powers of two k rather than by ten per zero, as an exact quotient at high precision can end in thousands of zeros. An exact quotient whose ideal form needs more than Precision digits now reports Rounded, as Mul does. Sqrt and Exp pad their results, so they are unchanged. Pow divides one by the power for a negative exponent, so 2**-3 is now 0.125 at any precision, as the specification says and issue cockroachdb#161 reports. The GDA suite compared divide results numerically, ignoring Rounded. They now all match exactly, so compare them with CmpTotal and check Rounded, except on a zero, as setExponent reports clamping its exponent as Rounded.
mw5h
left a comment
mw5h
left a comment
There was a problem hiding this comment.
I did an adversarial pass on this at eecfc2f, including a differential sweep of master vs. this branch vs. Python's decimal (~139k divisions across 9 precisions, all 8 rounding modes, 20k random quotients, subnormal grids) plus aliasing, Sqrt/Cbrt/Exp/Ln/Log10 regression checks, and a trial merge onto current master. Summary first, then the issues.
What checks out. No numeric value of any Quo result changes. All 49,022 exponent changes match Python exactly (master had 49,022 mismatches vs. Python, this branch has 0). Sqrt/Cbrt/Exp/Ln/Log10 are byte-identical to master, so the padDigits calls do their job. Aliased forms agree with the non-aliased result. The GDA divide suite passes with exponent-exact comparison. Reduce on big coefficients gets much faster (1000 trailing zeros: ~170µs → ~2.8µs). Merges cleanly onto e9c7a71 and the suite passes on the merged tree.
1. This changes CockroachDB's SQL output, and the commit message says otherwise
The commit message says downstream users reduce exact quotients by hand today. That is true of CUE but not of CockroachDB, which pins apd v3.2.3 and calls tree.DecimalCtx.Quo directly with no post-processing in pkg/sql/sem/eval/binary_op.go (EvalDivDecimalOp, EvalDivIntOp, EvalDivDecimalIntOp, EvalDivIntDecimalOp) and in the avg aggregate (aggregate_builtins.go). At CRDB's precision 20 this branch gives:
master: 30/6 = 5.0000000000000000000 1E-16/2 = 5.0000000000000000000E-17 6.1/2 = 3.0500000000000000000
PR: 30/6 = 5 1E-16/2 = 5E-17 6.1/2 = 3.05
CRDB logic tests encode the current form (logic_test/decimal lines 40-42 expect 5.0000000000000000000E-17; logic_test/aggregate 657-674 and 722-724 expect avg(k) = 5.0000000000000000000, avg(d) = 3.0500000000000000000). So bumping apd would change wire-visible results for every exact decimal division and exact avg(), and it reverses the stated rationale of #115 / 04d1483 (keep Precision digits for PG scale()/trim_scale() compatibility; PG gives 10::numeric/4 = 2.5000000000000000).
The code is spec-correct, so this is a policy call for the apd and CRDB owners rather than a bug: accept it and plan the CRDB side (CRDB cannot Reduce its way back to the old output; it would have to re-pad), gate it behind a Context option, or ship as a documented breaking change. Either way the commit message claim should be corrected and the behaviour change stated in the PR body.
2. Exact quotients are 2-4x slower
trimZeros is tuned for Reduce ("most coefficients have no trailing zero, one probe by 10 settles it"), but in Quo's exact branch the coefficient was just scaled by 10^(Precision-1) so it almost always ends in many zeros: the probe is wasted, then log2(n) big-int QuoRems follow, and above ~19 digits the uint64 fast path only kicks in late. ns/op, master → PR (-benchtime=20000x -count=3, stable):
| case | master | PR |
|---|---|---|
10/2 p=20 (CRDB-like) |
72 | 153 |
1/4 p=34 |
103 | 243 (+1 alloc) |
6.00/3 p=100 |
133 | 540 |
1/3 p=20 (inexact) |
70 | 72 |
999999999.5/1 p=9 (inexact carry) |
62 | 73 |
Exact division is the hot case for integer-typed SQL arithmetic and avg(). Options: try an exact division at the ideal exponent first (one QuoRem of the unscaled coefficients; if the remainder is zero and the quotient fits in Precision digits, done), or skip the by-10 probe from the Quo path and start at the largest k <= n. A BenchmarkQuo* in bench_test.go would keep this tracked. The +15% on the carry case is roundAddOne calling NumDigits twice.
3. Test gaps
trimZeros's big-int path (TrailingZeroBits bound, probe, power-of-two descent, fallthrough into the uint64 path,limit < zeros,limit > zeros) has no direct test;reduce.decTestruns at precision 9 so only the uint64 branch is exercised there. A 20k-case property test I ran passes, so it is correct today, just unprotected.- The subnormal flag fix is real but untested and unmentioned:
MinExponent=-999, p=3,1E-1000/1goes from1.0E-1000 subnormal, rounded(master) to1E-1000 subnormal(Python agrees with the PR). Pow(2, -3) -> 0.125(issue #161) is advertised in the commit message but has no test.TestQuoExponenthas no negative operands, zero dividend, partial-trim case (25E+2/1E+3,1/8at p=2 →0.13 inexact, rounded), orPrecision == 1(10/1 -> 1E+1 rounded).
4. Smaller things
Pow(1, -0.5)at p=5 moves from1.0000 inexact, rounded(spec/Python) to1 inexact. Root cause is pre-existing (Exp's early return for tiny|x|returns the bare constant1without padding; master'sPow(1, 0.5)already gives1); the old Quo's padding was hiding it for the negative case.Context.Quo's doc comment says nothing about the result exponent; since this changes a user-visible contract (exact → ideal exponent, inexact → Precision digits,Roundedwhen the ideal form does not fit) it should be documented there. ThepadDigitscomment says operations computed through Quo pad their result, but onlySqrtandExpdo.for k := int64(1) << bits.Len64(uint64(limit)) >> 1overflows forlimit >= 1<<62(unreachable today becauselimitis clamped toTrailingZeroBits()first; shifting bybits.Len64(...) - 1would be clearer). The uint64 loopfor n+4 <= limit && i%10000 == 0spins fori == 0with a large limit; callers guarantee nonzero, so latent only.- New
Roundedon exact-but-too-wide quotients (1000000000000/1at p=9) is spec-correct and matchesMul/Add, but a caller trappingRoundednow gets an error where it did not before. Worth a line in the PR body.
|
Need @rafiss to weigh in here. |
(see commit messages - please do not squash)
cc @rafiss :)