Repository navigation
perf: stop allocating unused aggregate slots for every group - #183
Merged
Merged
Conversation
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>
This was referenced Sep 23, 2026
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>
This was referenced Sep 27, 2026
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>
This was referenced Sep 30, 2026
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.
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
CompactAccumallocated three optional slot arrays — theCOUNT(DISTINCT),MEDIANandGROUP_CONCATvalue buffers — whether or not the query used any of them.sampleon a single-threadedGROUP BY user_idover an 11.15 GB / 75M row fixture with 5M groups: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
nullcan be shared by the whole query instead of allocated per group. Reads still seenull, 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:No regression on the paths that were already fast, same file:
GROUP BY country(8 groups)COUNT(*) WHERE country='JP'COUNT(DISTINCT user_id)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:
zig build test638/638 after rebase,zig fmt --checkclean.What this does not fix
Memory only — wall clock on high-cardinality
GROUP BYis unchanged. The parallel path replicates the group map per worker and merges serially, the same cliff #180 fixed for scalarCOUNT(DISTINCT).🤖 Generated with Claude Code