sql: stabilize decimal SQRDIFF with large offsets - #174680
Open
Alignyx wants to merge 1 commit into
Open
Conversation
Decimal SQRDIFF rounded a large initial value while updating its Welford mean, then multiplied the rounded residual by the original-scale delta. Distributed plans accumulated one such error per partial aggregation state. Center local inputs around the first value. During final aggregation, center each exact partial sum around one common offset before deriving its mean. This preserves translation-invariant differences without changing the distributed aggregate interface. Fixes cockroachdb#173065 Release note (bug fix): Fixed SQRDIFF, VARIANCE, and standard-deviation aggregates returning negative or plan-dependent results for high-precision DECIMAL values with a large common offset.
|
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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 #173065.
Summary
SQRDIFFon high-precisionDECIMALinputs with a large common offset couldreturn a negative value. The magnitude depended on how many local aggregation
states the physical plan created, so equivalent distributed scan and join plans
could produce different answers.
This change keeps the existing Welford aggregation and distributed intermediate
state, but performs the calculation in coordinates centered around a stable
offset. It also adds focused coverage for local accumulation, final partial-state
merging, and SQL execution with forced DistSQL.
Root cause
The decimal aggregate uses
tree.IntermediateCtx, whose precision is 25significant digits. For the reported value
99999999999999999999.9999999999, the first local Welford update previouslyproceeded as follows:
value - 0was rounded to 25 significant digits while computingdelta.-1E-10.1E20stored-1E10in the sum of squared differences.The bad state was therefore created by the first local input, before partial
aggregation state was serialized. Each local aggregation state contributed one
such error, which explains why the distributed scan and lateral join plans in
the issue returned different negative multiples of
1E10.The final decimal SQRDIFF aggregate had a related precision problem. It derived
each partial mean as
sum / countinIntermediateCtxand then subtracted theindependently rounded means. Two large partial means separated only in digits
beyond that context could become indistinguishable. For example, merging two
singleton states whose values differ by
1E-10returned zero instead of thecorrect
5E-21.Clamping a negative result to zero would hide the corrupted state and would
also be incorrect for inputs with a small, positive variance. Ten copies of the
reported value plus one value smaller by 2000 should produce
3636363.6363636363636, whereas the old implementation returned a negativeresult.
Fix
The local decimal accumulator now saves the first non-NULL input as an offset
and feeds
value - offsetinto the existing Welford recurrence. Variance andSQRDIFF are invariant under translation, so this preserves the result while
making the required precision depend on the spread of the inputs instead of
their absolute magnitude.
The final accumulator similarly chooses the first partial mean as a common
offset. For each partial state it computes
using
tree.ExactCtx, and only then divides by the count inIntermediateCtx. This retains low-order differences that would be lost bysubtracting separately rounded large means. The existing parallel Welford merge
then operates on those centered means.
The distributed
(sqrdiff, sum, count)intermediate representation, aggregatesignatures, planner mappings, result rounding, float implementation, and integer
delegation are unchanged. The additional decimal fields are included in memory
accounting and reset with the rest of the accumulator state.
Testing
Added
TestDecimalSqrDiffLargeOffset, covering:1E-10.Added the
regression_173065SQL logic test with forced DistSQL, covering bothidentical large values and the positive-variance case.
Validation included:
//pkg/sql/sem/builtins:builtins_testtarget;configurations;
distsql_agglogic suite;merge-join plans.
After the change, the reported all-equal inputs return zero in every tested plan,
while the positive-variance case retains its nonzero result.
Release note (bug fix): Fixed SQRDIFF, VARIANCE, and standard-deviation
aggregates returning negative or plan-dependent results for high-precision
DECIMAL values with a large common offset.