fix: stabilize variance aggregates at large offsets - #27549
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
fcd7648 to
913ab79
Compare
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head fb8c1a60617dbce1d694ad00154c0375d40636b6 against merge base 249a458ba567b26d1818fb0496b449135cb05ef2.
The rewritten head fixes the previously reported Decimal128 subtraction and one-row Chan intermediate cases, but two merge-bar failures remain.
[P1] Store a scaled second moment so a finite variance cannot overflow as M2
updateVarianceState and mergeVarianceState store the unnormalized M2, then divide only in getResult. M2 can exceed MaxFloat64 by the row count even when the SQL variance is finite. The current reordering at pkg/sql/colexec/aggexec/vardev2.go:264-269 only fixes the two-one-row example because its Chan weight is 0.5.
A minimal counterexample is two two-row partials with (mean=0, m2=0, count=2) and (mean=1.5e154, m2=0, count=2). The weight is 1, so the correction is 2.25e308 and M2 becomes +Inf; float64OfCheck is a no-op, and VAR_POP returns +Inf. The mathematical population variance is 2.25e308 / 4 = 5.625e307, which is finite. Processing [0, 0, 1.5e154, 1.5e154] through the resident Welford path reaches the same overflow on the fourth row, so the issue is not merge-only.
Keep a normalized/scaled moment (or otherwise apply count scaling before the overflowing product) so every finite representable VAR/STDDEV result remains finite in fill and merge. Add resident and partial-merge regressions with at least two rows per side; the existing one-row-per-side test cannot expose M2/result divergence.
[P1] Version the changed aggregate state across rolling CNs
This PR changes the three non-DECIMAL state vectors from (count, sum, sumsq) to (count, mean, M2) without changing their wire framing, and appends a fourth origin vector for DECIMAL inputs (vardev2.go:445-456). aggState.writeStateToBuf serializes each vector in order with no state-schema/version marker (aggState.go:424-443), while UnmarshalFromReader reads exactly the receiver factory's stateTypes.
During a rolling upgrade, an old worker's non-DECIMAL partial therefore decodes successfully on a new merge CN but is silently interpreted with the wrong semantics—for [1,2], old (2,3,5) becomes new mean=3, M2=5, yielding VAR_POP 2.5 instead of 0.25. DECIMAL partials fail framing outright because one side reads three vectors and the other four. The reverse new-worker/old-merge direction is equally incompatible.
Add an aggregate-state protocol capability and a legacy conversion/encoding path: a new receiver must recognize and convert (sum,sumsq) to (mean,M2), and new workers must emit legacy state to old merge peers (or distributed placement must be gated). Cover both mixed-version directions for numeric and DECIMAL partials. Local spill files only need current-format handling because they do not cross a binary generation.
fb8c1a6 to
586736c
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review on 586736c found two correctness blockers:
-
The unnormalized M2 state can still overflow although the requested SQL result is finite. For both the resident sequence [0, 0, 1.5e154, 1.5e154] and mergeVarianceState(0, 0, 2, 1.5e154, 0, 2), M2 becomes +Inf, while VAR_POP is the representable 5.625e307. float64OfCheck intentionally accepts infinity, so Flush returns +Inf. The existing one-row-per-side test only exercises weight 0.5 and misses this boundary. Please use an overflow-safe/scaled representation or formulation and cover resident plus merged states.
-
This changes the intermediate-state wire semantics without a protocol version or rollout gate. Numeric states keep the identical three vector types, but their meaning changes from (count, sum, sumsq) to (count, mean, M2). An old payload for values [1,2], namely (2,3,5), decodes successfully in the new executor and silently returns VAR_POP 2.5 instead of 0.25. Decimal changes from three vectors to four and fails framing in the opposite case. Mixed-version CNs can exchange these partials through group/mergegroup. Please add MORPC capability/version handling with legacy conversion or prevent mixed-version placement, and add both-direction compatibility tests.
ea43a09 to
482be5f
Compare
aptend
left a comment
There was a problem hiding this comment.
Re-review of exact head 0cea946. The rewrite closes my previous DECIMAL(38,20) subtraction-overflow case, and the normalized state plus v30/legacy protocol gate close the later M2-overflow and mixed-version state-layout findings. One finite-result intermediate overflow remains.
In pkg/sql/colexec/aggexec/vardev2.go:300, the carried normalized variance is multiplied before it is divided. With FLOAT64 rows [0, 2e154, 1e154], after two rows the mean is 1e154 and VAR_POP state is 1e308. The third row equals the mean, so its increment is zero and the exact next VAR_POP is 1e308 * 2 / 3 = 6.666666666666667e307, which is finite. The current left-associative expression evaluates 1e308 * 2 as +Inf first, then divides by 3, so BulkFill followed by Flush returns +Inf. I reproduced this with an operator-level regression; require.False(math.IsInf(result, 0)) fails.
Please evaluate the count ratio before multiplying (or use the scaled helper) and retain this counterexample as a regression.
| // before multiplying so a finite final variance does not overflow in an | ||
| // intermediate product. | ||
| increment := scaledProductQuotient(delta, value-nextMean, float64(nextCount)) | ||
| nextVariance := variance*float64(count)/float64(nextCount) + increment |
There was a problem hiding this comment.
This multiplication can overflow before the following division even when the stored VAR_POP is finite. Counterexample: [0, 2e154, 1e154]. Before the third row, variance=1e308, count=2, and increment=0; the exact next state is 6.666666666666667e307, but this expression computes 1e308*2 as +Inf and BulkFill/Flush returns +Inf. Please evaluate the ratio first or use scaledProductQuotient, with an operator-level regression.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 0cea946. This head does fix the previously reported Decimal128 deviation, unnormalized-M2 overflow, and mixed-version state-layout problems, but blocking numeric edges remain.
[P1] The old normalized variance is still multiplied before it is divided at pkg/sql/colexec/aggexec/vardev2.go:300. For the finite input [1.3e154, -1.3e154, 0], the first two rows produce a finite state variance of 1.69e308. The third row evaluates 1.69e308 * 2 before / 3, stores +Inf, and returns +Inf although VAR_POP is the representable 1.1266666666666666e308. A temporary focused test failed deterministically. Apply the same scaled product/quotient treatment to the existing-variance rescale and add this three-row regression.
[P1] A raw variance state cannot preserve a representable STDDEV when only the squared result exceeds float64. STDDEV_POP over [1e200, -1e200] is the finite 1e200, but updateVarianceState stores +Inf and getResult returns sqrt(+Inf). The same issue affects resident, DISTINCT, and merged execution. The shared VAR/STDDEV state needs an exponent-scaled representation or a STDDEV-safe equivalent, with resident and merge tests.
Performance: the unconditional Frexp/Ldexp path is now in every non-DISTINCT variance row. On this head, a focused M4 microbenchmark measured 8.58 ns/update versus 5.63 ns for the arithmetic path, about 52 percent slower in the recurrence itself. Keep the overflow-safe fallback, but use a finite direct-product fast path for normal values.
Please also revert or split the unrelated HNSW BVT edits. They remove the pre-delta f32 ANN oracle and add an unconditional sleep(20), deterministically increasing the suite while explicitly avoiding a stale-cache behavior unrelated to #27543. The existing variance-focused tests pass, but they do not cover the two counterexamples above.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-review after the rebase: the variance implementation and its tests are byte-for-byte unchanged from 0cea9464, so the blockers from my previous review remain on this head.
- The normalized update still multiplies before dividing (
variance * count / nextCount).[1.3e154, -1.3e154, 0]therefore returns+Infalthough the finite VAR_POP result is about1.1266666666666666e308. - STDDEV still stores raw variance and loses finite answers when only the square overflows:
STDDEV_POP([1e200, -1e200])returns+Inf, while the correct result is1e200. - The unconditional Frexp/Ldexp path remains roughly 52% slower in the focused update microbenchmark; use a finite direct arithmetic fast path with exponent scaling only near overflow/underflow.
- The PR still contains unrelated HNSW BVT edits; keep those out of this numerical fix.
Please address the finite-result counterexamples and add regression coverage before approval.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head a1dd839498bbb584644b4e4db3a7a7b08d4f6d13 against merge base 77c9ae710c991b219d1aabfb245cb9109dc5cd3c completed. The scaled normalized-variance representation closes the remaining finite-result overflow cases, including the multiply-before-divide counterexamples and finite STDDEV with unrepresentable squared variance. The common path now stays on direct arithmetic and only falls back to Frexp/Ldexp near overflow or underflow; the unrelated HNSW edits are gone. Decimal deviation handling, resident/DISTINCT/merge execution, sample scaling, and the v30 rolling-upgrade gate/legacy state layout have focused regression coverage. I found no remaining correctness, performance, compatibility, or unhappy-path blocker. Exact-head CI is green. Local focused binaries compiled but could not run in this checkout because the host lacks thirdparties/install/lib/libmo.dylib; that is an environment limitation, not a PR failure.
aunjgr
left a comment
There was a problem hiding this comment.
Approved at exact head a1dd839498bbb584644b4e4db3a7a7b08d4f6d13 against base 77c9ae710c991b219d1aabfb245cb9109dc5cd3c.
The normalized Welford/Chan representation restores translation stability without retaining an overflow-prone M2. The exponent sidecar closes finite VAR results whose intermediate square/weighted sum overflows and finite STDDEV results whose variance itself exceeds float64; ordinary values remain on the direct arithmetic fast path. Decimal inputs retain an exact origin and convert only deviations, including the exact-subtraction overflow fallback. Resident, DISTINCT, merge, population/sample, overflow, underflow, and large-offset cases are covered.
The aggregate-state layout change is capability-gated: pre-v30 remote execution uses the legacy three-vector implementation, the coordinator prevents incompatible multi-stage/shuffle placement, and remote validation fails closed. Current-format spill and mixed-version remote ownership are therefore separated. I found no remaining correctness, performance, compatibility, cleanup, or growth blocker.
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #27543
What this PR does / why we need it:
Replace the unstable sum-of-squares variance calculation with Welford online state and Chan parallel merging. DECIMAL aggregates retain an exact origin and convert only the deviation, preventing loss of small variance around large offsets.
Validation
go test ./pkg/sql/colexec/aggexec -count=1go test -race ./pkg/sql/colexec/aggexecgo test ./pkg/sql/colexec/group -count=1go vet ./pkg/sql/colexec/...aggexeccoverage: 79.0%