Skip to content

perf: stop allocating unused aggregate slots for every group - #183

Merged
melihbirim merged 1 commit into
mainfrom
perf/group-accum-optional-slots
Sep 23, 2026
Merged

melihbirim merged 1 commit into
mainfrom
perf/group-accum-optional-slots

Conversation

@melihbirim

Copy link
Copy Markdown
Owner

Replaces #181, which GitHub auto-closed when its base branch was deleted on merge of #180. Same branch, rebased onto main, now targeting main.

The problem

Every group's CompactAccum allocated three optional slot arrays — the COUNT(DISTINCT), MEDIAN and GROUP_CONCAT value buffers — whether or not the query used any of them.

sample on a single-threaded GROUP BY user_id over an 11.15 GB / 75M row fixture with 5M groups:

2134 of 3029 samples (70%)  engine.zig  CompactAccum.init
 861 of 3029 samples (28%)  hash_map    getOrPut

That run held 2810 MB for a 10-character key and one counter per group — 562 bytes per group.

The fix

A slot array that will only ever be read as null can be shared by the whole query instead of allocated per group. Reads still see null, and the branches that write one only run when the matching aggregate exists, in which case the array is allocated per group exactly as before. No use site changed.

GROUP BY user_id, 5M groups, 12 threads:

memory
before 3395 MB
after 2188 MB

No regression on the paths that were already fast, same file:

query after
GROUP BY country (8 groups) 2.27s / 55 MB
COUNT(*) WHERE country='JP' 2.01s / 53 MB
COUNT(DISTINCT user_id) 5.49s / 235 MB

Correctness

The risk is a shared array being written. Verified against DuckDB on a 2M-row file with all three optional kinds in one query, parallel and single-threaded:

SELECT country, COUNT(*), MEDIAN(amount), COUNT(DISTINCT device), GROUP_CONCAT(device)
  csvql   BR,250000,500.01,3,"tablet,desktop,..."
  duckdb  BR,250000,500.01,3

SELECT MEDIAN(amount), COUNT(DISTINCT device), COUNT(DISTINCT country)
  csvql   499.995,3,8
  duckdb  499.995,3,8

zig build test 638/638 after rebase, zig fmt --check clean.

What this does not fix

Memory only — wall clock on high-cardinality GROUP BY is unchanged. The parallel path replicates the group map per worker and merges serially, the same cliff #180 fixed for scalar COUNT(DISTINCT).

🤖 Generated with Claude Code

Every group's CompactAccum allocated three optional slot arrays — the
COUNT(DISTINCT), MEDIAN and GROUP_CONCAT value buffers — whether or not
the query used any of them. `sample` on a single-threaded
GROUP BY user_id over an 11.15 GB / 75M row CSV with 5M groups put 2134
of 3029 stack samples (70%) in CompactAccum.init, and the run held
2810 MB for what is a 10-char key and one counter per group: 562 bytes
per group.

A slot array that will only ever be read as null can be shared by the
whole query. Reads still see null, and the branches that write one only
run when the matching aggregate exists, in which case the array is
allocated per group exactly as before — so no use site changes.

GROUP BY user_id, 5M groups, 12 threads:

  before  3395 MB
  after   2188 MB

Unchanged on the paths that were already fast, same file:

  GROUP BY country (8 groups)  2.27s / 55 MB
  COUNT(*) WHERE country='JP'  2.01s / 53 MB
  COUNT(DISTINCT user_id)      5.49s / 235 MB

This is memory only. High-cardinality GROUP BY is still slower than it
should be because the parallel path replicates the group map per worker
and merges serially; that is the same cliff #180 fixed for scalar
COUNT(DISTINCT) and it still needs its own change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@melihbirim
melihbirim merged commit 8fe1654 into main Sep 23, 2026
13 checks passed
@melihbirim
melihbirim deleted the perf/group-accum-optional-slots branch September 23, 2026 12:44
melihbirim added a commit that referenced this pull request Sep 23, 2026
…nd (#187)

Covers the COUNT(DISTINCT) benchmark against Python, Node, Rust and
DuckDB on an 11.15 GB / 75M row file, where csvql started out last by a
wide margin, and the replicate-and-merge bug that caused it.

All numbers are the ones measured for #180, #182, #183 and #184, with
best-of-3 used consistently across every tool. The open items in #185 and
#186 are linked rather than left out.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
melihbirim added a commit that referenced this pull request Sep 28, 2026
…) (#191)

Variance was computed as sum_sq/n - mean*mean. Both terms are large and
nearly equal once the values are large relative to their spread, so the
subtraction cancels the answer away. On a column of unix timestamps it
was 84% low; at 1e9 it returned exactly 0, silently, for VARIANCE,
STDDEV, VARIANCE_SAMP and STDDEV_SAMP alike.

Three consecutive integers have a sample variance of exactly 1 at any
magnitude:

  mean~1e3   0.9999999999417923   ->  1
  mean~1e5   0.9999990463256836   ->  1
  mean~1e7   1.0078125            ->  1
  mean~1e9   0                    ->  1
  mean~1.7e9 0                    ->  1

100 unix timestamps one second apart, true sample variance 841.6667:

  before  1551.5151515151515
  after    841.6666666666666   (matches DuckDB bit for bit)

Now uses Welford's online algorithm, with Chan's parallel merge to
combine per-worker accumulators, so the two large terms are never formed.
The running mean is a new per-slot array allocated through the
OptionalSlots mechanism from #183, so queries without a variance or
stddev aggregate share one zero array and pay nothing per group.

Found by bench/query_fuzz.sh --seed 4242, which flagged a 1-unit
difference on VARIANCE(id) and turned out to be the mild end of it. That
seed now runs 1500 queries with zero divergence; the regression file it
dumped is committed alongside.

On a 1.2M row fixture whose groups are exact arithmetic sequences, the
true sample variance is exactly 120000400000. csvql now returns that
exactly; DuckDB returns 120000400000.0163.

bench/verify_correctness.sh 103/103, zig build test 640/640.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant