Conversation
Large common offsets consume precision in the decimal running mean and can make SQRDIFF inaccurate for reordered inputs. Center direct inputs and partial aggregate sums before updating the mean. Preserve the additional decimal memory accounting and reset state between groups. Add regressions for all orders of a two-value multiset in the direct and partial-state paths, translated inputs, NULLs and Reset. They fail on the unmodified baseline and pass with the change; the builtins suite passes. Production patch generated with Codex CLI through MetamorphicRepair. Fixes cockroachdb#101588 Release note (bug fix): Improved SQRDIFF accuracy for integer and decimal inputs with large common offsets. Changes in input order or the combination of partial aggregates could previously produce inaccurate results for these values.
|
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 branch has not been deployed
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.
SQRDIFFcan lose accuracy when its inputs have a large common offset and a much smaller spread. In #101588, the same four integer values produce different answers depending on the order selected by the join. For two copies each of856809699799498753and856809699799531521, the squared deviations sum to1073741824.The decimal accumulator already uses Welford's update, but it maintains a mean near the original large values. Updating that mean in a finite-precision context loses digits that are needed for the small differences.
This change centers the inputs around the first value before accumulating. Translation preserves squared deviations, and the running mean now describes the small spread instead of the large offset. The integer implementation also benefits because it delegates to the decimal accumulator.
The partial-state combiner needs the same treatment. It uses the first partial mean as a common origin and computes
sum - origin * countwith exact multiplication and subtraction before dividing. That avoids first rounding two large means and then subtracting them. Both accumulators account for the additional decimal storage and clear it inReset.The regression runs all six orders of the two-value multiset through the direct and partial-state accumulators, with positive, zero and negative offsets. It also exercises NULL input and reuse after
Reset. This improves the reported large-offset case; finite-precision arithmetic still applies to general decimal inputs.Validation
bazel test //pkg/sql/sem/builtins:builtins_test --test_arg=-test.run=^TestSqrDiffLargeOffset$passed.8812064afailed in both the direct and partial-state paths; one observed result was1073741823.9989077333instead of1073741824.bazel test //pkg/sql/sem/builtins:builtins_testpassed, including the added regression.The package tests exercise the partial-state implementation directly. They are not a separate multi-node SQL or exhaustive extreme-DECIMAL/window-function validation.
Fixes #101588
Release note (bug fix): Improved SQRDIFF accuracy for integer and decimal inputs with large common offsets. Changes in input order or the combination of partial aggregates could previously produce inaccurate results for these values.