feat(sql): add kurtosis and skewness aggregates

[brunocalza]

Summary

Adds kurtosis and skewness methods addressing Gap 4 of questdb/roadmap#120. To be precise, the following functions are added

Function	Description
kurtosis / kurtosis_samp	Sample excess kurtosis
kurtosis_pop	Population excess kurtosis
skewness / skewness_samp	Sample skewness
skewness_pop	Population skewness

Example

SELECT skewness(price), kurtosis(price)
FROM trades
WHERE symbol = 'BTC-USD'
SAMPLE BY 1h;

Documentation: questdb/documentation#463

Implementation details

• The state for each group keeps a running mean and the central moments M2, M3 (and M4 for kurtosis) plus a count, updated per row with Pébay's one-pass online algorithm (an extension of Welford's)
• Partial aggregates merge with the matching pairwise-combine formulas, so the functions support parallel GROUP BY execution
• The streaming approach avoids a second pass over the data and is numerically more stable than the naive sum-of-powers formula.

Conventions

• Sample skewness returns null for fewer than 3 values; sample kurtosis for fewer than 4
• The sample variants apply the standard bias correction (Fisher's definition)
• Population variants return null for an empty group
• All variants return null when every observation is equal (zero variance), since the
  statistic is undefined

Testing

• Unit tests using DuckDB as a reference
• A script outside this PR was used to compare the values we compute with this implementation and scipy

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8e3b985d-82a4-4616-be23-941ca252a769

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[bluestreak01]

Thanks for this — the math is sound. I re-derived the Pébay online update, the pairwise combine, and all four final statistics by hand, and cross-checked the merge() combine against a single-pass reference over 2000 randomized 3-way splits (max abs diff ~1e-13, i.e. pure float roundoff). I found no correctness, concurrency, or resource bug. The implementation faithfully mirrors the existing AbstractStdDevGroupByFunction family. The requests below are about test coverage (one of them important) plus two quick PR-hygiene fixes.

Must address

1. The merge() Pébay combine — the most complex new code — has zero test coverage

This is the main one. In both KurtosisParallelGroupByTest and SkewnessParallelGroupByTest the only non-null values are at x BETWEEN 1500 AND 1509. With CAIRO_SQL_PAGE_FRAME_MAX_ROWS = 64, those 10 rows (0-indexed 1499–1508) all land inside a single page frame (frame 23, rows 1472–1535). So exactly one worker partial is ever non-empty, and every merge() call hits only the nB == 0 early-return or the nA == 0 straight-copy fast path. The actual combine branch — both nA > 0 and nB > 0, i.e. the delta3/delta4 M3/M4 formulas in AbstractKurtosisGroupByFunction.merge() / AbstractSkewnessGroupByFunction.merge() — never executes in any test, serial or parallel.

The combine math is correct (I verified it numerically), but it's the riskiest code in the PR and currently nothing guards it against a future regression.

To exercise it, the non-null values for a given group must span multiple page frames so that more than one worker partial accumulates real data for that group and the partials actually get combined. Please look at how the other group-by functions test parallel execution:

• core/src/test/java/io/questdb/test/griffin/engine/functions/groupby/StdDevParallelGroupByTest.java — testParallelKeyedSparseNulls shows the keyed GROUP BY grp shape (note: spread the non-null rows across frames, not 10 consecutive rows).
• core/src/test/java/io/questdb/test/cairo/fuzz/ParallelGroupByFuzzTest.java — the canonical pattern: run the aggregate on random data over many groups/frames and assert the parallel result equals the same query with CAIRO_SQL_PARALLEL_GROUPBY_ENABLED = false. That parallel-vs-serial cross-check is the robust way to validate the merge.

2. Bare kurtosis() / skewness() aliases are untested

KurtosisGroupByFunctionFactory / SkewnessGroupByFunctionFactory are separate classpath-scanned classes that only override getSignature() and inherit behavior from the _samp factory. The StdDev family ships StdDevGroupByFunctionFactoryTest for exactly this; this PR drops the equivalent, so if an alias were accidentally pointed at the _pop factory nothing would catch it. A small test asserting kurtosis(x) == kurtosis_samp(x) (and likewise skewness) on the same data covers it.

3. No keyed GROUP BY test anywhere (serial or parallel)

Every query is a whole-table single-group aggregate; the *MultiColumn tests are multi-column / single-group. The per-key valueIndex-offset isolation and multi-entry map path are never exercised. A keyed test producing more than one output row (ideally folded into #1) covers it.

4. Description claims a null-vs-NaN distinction that QuestDB can't surface

The body says "Population variants return null only for an empty group" and "When all observations are equal … the result is NaN". In QuestDB, Double.NaN is the DOUBLE NULL sentinel — both branches return Double.NaN and both render as null, so there's no observable difference. Please reword.

5. Missing labels

feat(sql) adding aggregates should carry at least SQL and New feature.

Nice to have (non-blocking)

• SAMPLE BY fill is untested. The _samp variants deliberately set getSampleByFlags() = SAMPLE_BY_FILL_ALL (routing through setDouble/setNull), but no SAMPLE BY … FILL(…) test exercises it. (Heads-up: like the StdDev family, FILL(LINEAR) will throw at cursor time because no interpolation method is implemented — pre-existing, just be aware.)
• No numerical-stability / large-n test. 4th-order moments have a much larger dynamic range than variance — the whole point of Pébay is stability — yet the largest computed sample is long_sequence(4). Compare StdDevSampleGroupByFunctionFactoryTest.testStddevSampHugeValues (1M rows).
• Test anti-pattern (×4): the testXxxAllNull cases (line 34 of each factory test) are a lone single-statement assertQuery(...) wrapped in assertMemoryLeak(() -> …) + .noLeakCheck(). The builder leak-checks itself — drop the wrapper and .noLeakCheck(). (It mirrors the StdDev convention, hence non-blocking.)
• Perf opportunity (consistent with StdDev, not a regression): no computeKeyedBatch override, so the parallel keyed path pays per-row interface dispatch that the AvgDoubleGroupByFunction pattern avoids. The per-row aggregate() itself is optimal (one division/row; count via get+put is actually leaner than StdDev's get+addLong).
• Stray blank line at KurtosisGroupByFunctionFactory.java:32 (absent in the Skewness sibling). Infinity inputs untested; the two abstract bases differ only by the M4 slot and could share a base.

Net: solid implementation, correct numerics — the blocking ask is real test coverage for the parallel merge() combine (plus the alias/keyed gaps), then the description/label tidy-up.

[brunocalza]

Thanks for the review, @bluestreak01 . I have addressed the blocking items and most of the nice-to-haves.

Must address

1. merge() combine coverage. Added testParallelGroupByKurtosis and testParallelGroupBySkewness to ParallelGroupByFuzzTest

   I left the Assume.assumeTrue(enableParallelGroupBy) gate off from these tests, so the query can be executed serially and in parallel and divergences can be found.

2. Bare aliases. Both fuzz tests assert kurtosis(x)/skewness(x) against the _samp variant on the same data, so an alias accidentally wired to _pop would fail.

3. Keyed GROUP BY. Each fuzz test adds a SELECT key, ... `GROUP BY key ORDER BY key query over 5 keys, exercising the per-key map merge that the whole-table queries don't.

4. PR description of null vs. NaN. Reworded the Conventions section.

5. I don't have permissions to add those labels

Nice to have

• Large-n/numerical stability. Added testKurtosisSampHugeValues and testSkewnessSampHugeValues
• Test anti-pattern (×4). Dropped the redundant assertMemoryLeak(...) wrapper and .noLeakCheck() from the four AllNull tests; they now use the bare self-leak-checking assertQuery(...).returns(...).
• Stray blank line. Removed at KurtosisGroupByFunctionFactory.

The other suggested nice-to-haves I have left consistent with the stddev/var family (happy to change if you'd prefer).

[bluestreak01]

Approving after a full independent review pass.

Correctness (verified independently):

• Online Pébay/Welford update and the pairwise M2/M3/M4 combine match the reference formulas exactly (signs included). Hand-traced n=1, n=2, n=4 cases against the asserted test values.
• All four final statistics are correct: skewness_pop, kurtosis_pop, adjusted Fisher-Pearson skewness_samp, and kurtosis_samp.
• Division-by-zero guards are complete: n<3 (skew_samp), n<4 (kurt_samp), and m2==0.0 for all four. M2 is provably >=0 and stays exactly 0 across merges of all-equal data, so the zero-variance branch fires correctly even in parallel.

NULL handling: the int/long->double cast maps the sentinel NULL to NaN and isFinite() filters it; exercised by the INT *MultiColumn tests. Empty/all-null groups return null.

Concurrency/resources: no per-function mutable state; accumulators live in MapValue; the nA==0/nB==0 fast paths avoid the empty-partial NaN-poisoning class of bug (cf. #7160). No native allocation, no leak surface.

Tests: the parallel merge-combine path is genuinely exercised by the new fuzz tests (~36 frames x 5 keys, serial-vs-parallel cross-check).

Non-blocking nits left to discretion: missing SQL / New feature labels; the two dedicated *ParallelGroupByTest classes only hit the merge fast paths (combine is covered by the fuzz tests); FILL(LINEAR) on the _samp variants throws at cursor time (pre-existing, consistent with the StdDev family); the two abstract bases could share code.