Repository navigation
Implement numerically stable M2 aggregation in hash-groupby - #24492
Merged
Merged
Conversation
bdice
approved these changes
Oct 7, 2026
bdice
left a comment
Contributor
There was a problem hiding this comment.
I'm okay with the slight performance regressions in exchange for the numerical stability. I think this is a worthwhile change.
4 tasks done
davidwendt
reviewed
Oct 8, 2026
davidwendt
reviewed
Oct 8, 2026
Add exact group-size boundary cases, fixed low cardinalities, and a deterministic hot-group distribution for M2, variance, and standard deviation. Reuse the existing timed aggregation harness and preserve the original random workloads.
Replace ordinary hash groupby's raw-moment M2 with a Welford/Chan state reduction over the existing CSR group and chunk schedule. Produce M2 and valid counts together and reuse them for variance/std finalization. Preserve independent SUM/MEAN/SUM_OF_SQUARES requests, result resource placement, and streaming groupby's existing raw-moment contract. Add numerical regression coverage for large offsets, reduction boundaries, sliced dictionaries, null keys/values, ddof, non-finite inputs, extreme constants, and repeated or reordered requests.
wence-
force-pushed
the
wence/fea/stable-m2
branch
from
October 9, 2026 09:14
8acb9ae to
56375d8
Compare
wence-
force-pushed
the
wence/fea/stable-m2
branch
from
October 9, 2026 09:38
56375d8 to
6e79722
Compare
wence-
commented
Oct 12, 2026
Contributor
Author
|
/merge |
ttnghia
reviewed
Oct 12, 2026
mhaseeb123
reviewed
Oct 12, 2026
mhaseeb123
approved these changes
Oct 13, 2026
Contributor
Author
|
Note, since a lot of the decisions here are the same as in #24489, let's get happy with that and merge it before finishing this. |
Contributor
Author
|
Updated based on the comments on #24489 |
wence-
force-pushed
the
wence/fea/stable-m2
branch
from
November 5, 2026 12:11
474242a to
ca7d4a6
Compare
Build the value profile once so every column uses it instead of a moved-from builder, and pass bounds of the distribution's own type for values and keys.
PointKernel
approved these changes
Nov 6, 2026
Matt711
approved these changes
Nov 9, 2026
3 tasks
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.
Description
Previously, the hash-groupby aggregation computed the
M2aggregation in two passes asThis is numerically very unstable and easily suffers from catastrophic cancellation such that although this is supposed to be a non-negative value (since it is$(x - \bar{x})^2$ ), we can easily end up with a negative
M2.For full column reductions for
M2we already use a numerically stable one-pass approach. Previously we did not do that for the hash-groupby since it requires 128bit CAS loops with some extra state. Now that hash-groupby has moved to HashCSR we can compute in one pass using the same one-pass approach. Note that the old approach is still used in the streaming-groupby code.This is, unfortunately not a complete slam-dunk in terms of performance. Throughput degrades somewhat on B200 (see nvbench comparisons below). However, I argue that the performance drop is small enough that this is worthwhile so that it is numerically safe to use hash-groupby in more cases.
H100 Benchmark comparisons
groupby_m2_var_std
[0] NVIDIA H100 80GB HBM3
B200 Benchmark comparisons
groupby_m2_var_std
[0] NVIDIA B200
Checklist