{{ message }}
Add skewPopStable, skewSampStable, kurtPopStable, kurtSampStable aggregate functions#108685
Open
coderashed wants to merge 10 commits into
Open
Add skewPopStable, skewSampStable, kurtPopStable, kurtSampStable aggregate functions#108685coderashed wants to merge 10 commits into
coderashed wants to merge 10 commits into
Conversation
The else branch used `source.mean + delta * (na / nf)` which is algebraically incorrect. The right formula is `mean + delta * (nb / nf)` (Welford/Chan combined-mean identity: mean_new = mean_A + delta * n_B / n). With the wrong sign, each chained `mergeWith` call stored a corrupted mean, causing wrong deltas in subsequent merges. This manifested only under parallel aggregation (max_threads > 1) where `mergeWith` is called in a chain. Single-threaded mode was unaffected because m2/m3/m4 read the delta before mean is updated within a single call. Removes the `SETTINGS max_threads=1` workaround from the merge-correctness test (c) in 04412_skew_kurt_stable.
…tric split, and two-element edge cases
…merge test for skew/kurt stable
…ble, kurtSampStable
…table, kurtSampStable Each registration now includes a description, syntax, argument list, return value description, examples with verified output, version (`IntroducedIn` 26.7), and category, matching the convention used by all other functions in the file.
…both update and merge formulas
…ueryFuzzer aggregate function list The other stable variants (`stddevPopStable`, `varSampStable`, etc.) were already present; these four were overlooked.
alexey-milovidov
added
the
can be tested
Contributor
|
Workflow [PR], commit [540a7e5] Summary: ❌
AI ReviewSummaryThis PR adds numerically stable aggregate variants for population/sample skewness and kurtosis, wires them into source-level documentation and the query fuzzer, and adds focused stateless coverage for translation invariance, parity with existing formulas, merge behavior, and edge cases. I did not find any correctness, safety, or compatibility issue that warrants an inline review comment. Final VerdictStatus: ✅ Approve |
Contributor
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered by tests: 192/207 (92.75%) | Lost baseline coverage: none · Uncovered code |
coderashed
added a commit
to coderashed/ClickHouse
that referenced
this pull request
Jun 27, 2026
Add the query shape the AST fuzzer aborted on in the `amd_msan` stress test of ClickHouse#108685 (multi-column projection with a correlated scalar subquery over a `GROUPING SETS` key under `WITH TOTALS`), scaled down and deterministic, so the regression test ties directly to the reported failure.
coderashed
added a commit
to coderashed/ClickHouse
that referenced
this pull request
Jun 27, 2026
A correlated scalar subquery referencing an outer `GROUP BY` key that becomes `Nullable` via `group_by_use_nulls` (`GROUPING SETS` / `ROLLUP` / `CUBE` / `WITH TOTALS`) aborted with: Logical error: Unexpected return type from toString. Expected String. Got Nullable(String) The `group_by_use_nulls` nullability walk in `resolveExpressionNode` stops at the subquery's own query scope, so a correlated column kept its non-`Nullable` type while decorrelation fed in the real `Nullable` column, mismatching a baked function result type (`toString` -> `String` vs `Nullable(String)`). Continue the walk past a subquery's query boundary for correlated columns and convert the column to `Nullable` in place, preserving the node identity shared with the subquery's correlated-columns set (which the planner matches by identity). Discovered by the AST fuzzer in the `amd_msan` stress test: https://github.com/ClickHouse/ClickHouse/actions/runs/28286443407/job/83839128485?pr=108685 Related: ClickHouse#108685
coderashed
added a commit
to coderashed/ClickHouse
that referenced
this pull request
Jun 27, 2026
Add the query shape the AST fuzzer aborted on in the `amd_msan` stress test of ClickHouse#108685 (multi-column projection with a correlated scalar subquery over a `GROUPING SETS` key under `WITH TOTALS`), scaled down and deterministic, so the regression test ties directly to the reported failure.
Author
|
The stress test CI failure here is an existing bug unrelated to this PR's changes — it's being fixed in #104350. This PR should be clear to merge once that lands. |
Author
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
2 participants
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.
Adds four numerically stable aggregate functions for skewness and kurtosis:
skewPopStableskewSampStablekurtPopStablekurtSampStableMotivation
The existing
skewPop,skewSamp,kurtPop, andkurtSampfunctions use a naive two-pass formula that suffers catastrophic cancellation when the mean is large relative to the standard deviation. For example, power or voltage telemetry (mean ~8000W, σ ~50W), Unix timestamps, or any metric with a large DC offset will produceNaNor wildly incorrect results from the naive variants.Algorithm
Based on Pébay, Terriberry, Kolla, Bennett (2016) "Numerically stable, scalable formulas for parallel and online computation of higher-order multivariate central moments with arbitrary weights", Computational Statistics 31:1305–1325, DOI 10.1007/s00180-015-0637-z.
add) — Corollary 3.2: single-pass recurrence for M2, M3, M4. Update order is mandatory: M4 → M3 → M2 (each reads the not-yet-updated lower moment).mergeWith) — Proposition 3.1: formulas for combining two partitions. Keeps all intermediate values in the coordinate frame of the data, so accuracy is independent of the absolute offset.The
-State/-Mergecombinators work correctly for distributed aggregation.Correctness
All four functions return
nanon empty input, singleton input, or constant input (zero variance). Verified against analytical exact values:skewPop = 0,kurtPop = (9N²−21)/(5N²−5)skewPop = 2/√3,kurtPop = 7/3(exact rational)Translation invariance is verified in the test suite at a
1e9offset, where the naive variants returnNaNor garbage and the stable variants match the unshifted result.Changelog category (leave one):
Changelog entry:
Adds
skewPopStable,skewSampStable,kurtPopStable, andkurtSampStableaggregate functions — numerically stable variants of the existing skewness and kurtosis functions that remain accurate even when the mean is large relative to the standard deviation.