perf: parallelize high-cardinality mixed DISTINCT aggregation - #27693
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
3842ade to
7ec8848
Compare
LeftHandCold
left a comment
There was a problem hiding this comment.
Reviewed the exact head b1118eb. Checked arena relocation and link preservation, cached Inserter merge behavior, target/source DISTINCT deduplication, COUNT(DISTINCT) shuffle eligibility and single-stage safeguards. Focused arenaskl tests and diff-check passed; no reachable correctness or normal-aggregation regression blocker found. The remaining Ubuntu/SCA/BVT checks were still running at review time.
### What this changes Make saved-argument DISTINCT preflight adaptive instead of forcing either quadratic scans or hashing every row: - split DISTINCT and non-DISTINCT admission into separate functions, so ordinary aggregates do not clear or retain the DISTINCT hash table stack frame; - keep up to 8 exact `(target group, DISTINCT arguments)` representatives and use exact equality only; - initialize the fixed 512-slot open-addressing table only when the ninth unique tuple arrives, bounding high-cardinality admission to expected O(B) for the 256-row work unit; - delay single-column payload access until a tuple is actually admitted, so duplicate-heavy fixed and varlen inputs are faster than the old scan; - retain collision-safe canonical equality for NULL filtering, const vectors, offsets, multi-column boundaries, and floating-point signed zero; - add a direct non-DISTINCT single-argument loop so splitting the paths does not regress ordinary saved arguments. The implementation keeps regular and DISTINCT admission separate and pays the bounded hash-table cost only after the exact low-cardinality path overflows. A full aggregate-framework rewrite would be disproportionate for this bounded 256-row preflight unit. ### arenaskl correctness fix A reused `arenaskl.Inserter` could miss equality when the requested key was exactly its cached base-level successor. The PR now checks that successor before leaving the cached-splice path and adds `Inserter.AddWithPlan` so sorted planned publications can safely retain the splice cache. Fixes #27687. Fixes #27691. Related to #27672; the mixed-aggregation query optimization remains in #27693. ### Performance Pinned to one CPU, `-cpu=1`, 256 rows, median of 3 runs (focused non-DISTINCT fixed result uses 5 runs at 2s): | input | cardinality | base | head | result | |---|---:|---:|---:|---:| | INT64 DISTINCT | 1 | 7.813 us | 6.987 us | 10.6% faster | | INT64 DISTINCT | 2 | 10.451 us | 9.959 us | 4.7% faster | | INT64 DISTINCT | 8 | 25.741 us | 25.561 us | neutral / 0.7% faster | | INT64 DISTINCT | 9 | 27.920 us | 10.896 us | 2.56x faster | | INT64 DISTINCT | 256 | 648.647 us | 14.664 us | 44.2x faster | | 1 KiB VARCHAR DISTINCT | 1 | 12.282 us | 10.770 us | 12.3% faster | | 1 KiB VARCHAR DISTINCT | 2 | 16.562 us | 15.863 us | 4.2% faster | | 1 KiB VARCHAR DISTINCT | 8 | 44.797 us | 43.617 us | 2.6% faster | | 1 KiB VARCHAR DISTINCT | 9 | 48.153 us | 28.928 us | 1.66x faster | | 1 KiB VARCHAR DISTINCT | 256 | 999.855 us | 46.526 us | 21.5x faster | | INT64 non-DISTINCT | n/a | 7.632 us | 7.542 us | 1.2% faster | | 1 KiB VARCHAR non-DISTINCT | n/a | 24.638 us | 24.605 us | neutral | All focused cases remain at 0 B/op and 0 allocs/op. The existing 65,536-row `BenchmarkCountDistinctSavedArguments` remains about 48-49 ms/op. Stack frames from the compiled test binary: - base combined path: 12,832 bytes; - head non-DISTINCT single path: 12,448 bytes; - head low-cardinality DISTINCT path: 8,400 bytes; - high-cardinality hash helper: 5,296 bytes in its separate frame. ### Regression coverage - true 256-unique-candidate hash-table test plus a complete second duplicate pass; - threshold transition at 8/9 unique tuples; - end-to-end adaptive multi-column DISTINCT test with offsets, NULL, unmatched groups, two groups, repeat preflight, publication, and result cardinality; - non-DISTINCT multi-argument preflight remains publication-free; - cached Inserter sorted-overlap regression checks duplicate rejection and final cardinality; - benchmark matrix covers cardinality 1, 2, 8, 9, and 256 for fixed/varlen plus non-DISTINCT controls. Exact package coverage comparison (`pkg/common/arenaskl` + `pkg/sql/colexec/aggexec`): - base: 10,520 / 13,316 statements = 79.002704%; - head: 10,669 / 13,483 statements = 79.129274%. ### Validation - full normal tests for both packages; - full race tests for both packages; - `go vet` for both packages; - deterministic CGo compile/link/runtime environment; - pinned-CPU benchmark matrix; - `git diff --check`; - rebased onto latest `origin/main` (`e8332cb8cc`).
b1118eb to
25d0440
Compare
aptend
left a comment
There was a problem hiding this comment.
Reviewed exact head 0a232e7, including the full diff, prior review, arena/accounting failure paths, DISTINCT merge state, planner decisions, and compile topology. Build, vet, owning-package tests, targeted race repetitions, and the new benchmark pass locally. One grouping-sets correctness blocker remains.
| } | ||
| if highestNDV < ShuffleThreshHoldOfNDV { | ||
| minimumGroupNDV := float64(ShuffleThreshHoldOfNDV) | ||
| if distinctStateShuffle { |
There was a problem hiding this comment.
[P1] Exclude inactive grouping-set keys from this shuffle path. For a grouping-set branch, GroupingFlag[i] == false means Group replaces that key with the rollup constant, but constructShuffleArgForGroup hashes the raw child column before Group performs that replacement. The new distinctStateShuffle threshold can therefore enable a topology that was previously rejected. For example: SELECT region_id, SUM(x), COUNT(DISTINCT user_id) FROM t GROUP BY GROUPING SETS ((region_id), ()) with 1M rows, about 100 regions, and high user_id NDV. In the () branch the planner chooses raw region_id, distributes one logical grand-total group across owners, then every owner changes the key to the same rollup constant locally; there is no downstream MergeGroup, so the query can emit duplicate grand-total rows with partial SUM/COUNT results. A direct counterexample in TestDetermineShuffleForGroupByAccountsForCountDistinctState using GroupingFlag = []bool{false} currently observes Shuffle == true. Please select only active grouping keys here (or disable the path for inactive grouping-set branches) and cover the empty grouping set.
There was a problem hiding this comment.
Fixed in 19ee024. determineShuffleForGroupBy now selects the distribution key only from active GroupingFlag entries; an empty grouping set with no active key leaves shuffle disabled, while a mixed branch still shuffles on its active key. Added deterministic coverage for both cases in TestDetermineShuffleForGroupByAccountsForCountDistinctState. Planner owning-package and compile shuffle consumer tests pass.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review completed on exact head 0a232e7. No blocking correctness, resource-lifecycle, performance, or unhappy-path issue found. Planner NDV/state guards, single-owner topology handling, arena relocation, allocator-pressure fallback, and DISTINCT deduplication are covered. Focused arenaskl, mpool, planner, compiler, and aggexec tests passed. The pr-size-label failure is an unrelated GitHub API TLS failure.
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 19ee0241dd22453de010225225d937988d642648 against merge-base ff4270c844c4b630cf1d921da813ba119d5b5e89. I re-read the complete diff, all reviews/inline comments/replies, and the remaining unresolved thread, then checked the one-commit delta from my prior reviewed head 0a232e7492b040b1984b25caf60f7af49c3e15a9. The grouping-sets blocker is closed: inactive keys are excluded before distribution-key selection, an empty grouping set keeps the merge topology, and a mixed branch can select only an active key. Owning-package list/build/vet/full tests, 50 race repetitions across arena/preflight/planner/compile paths, and the relevant benchmarks pass. No blocking issue found.
aunjgr
left a comment
There was a problem hiding this comment.
Approved at exact head 19ee0241dd22453de010225225d937988d642648 against base ff4270c844c4b630cf1d921da813ba119d5b5e89.
The planner now costs mergeable COUNT(DISTINCT) exact state separately from final group cardinality, applies conservative filtered-NDV/state-ratio/owner/key guards, clears stale shuffle state for non-mergeable DISTINCT aggregates, and avoids a one-owner shuffle at compile time. Shuffling by an active logical group key gives every exact state one owner and preserves results; inactive grouping-set keys and empty grouping sets correctly retain MergeGroup.
Arena growth allocates the replacement under the existing account while the old arena remains charged, clamps speculative capacity to MPool’s real single-allocation ceiling, retries only classified capacity pressure at the linear fallback, copies the valid arena prefix, rebinds relative links, and frees the old owner only after publication. Failure leaves the old list unchanged. Cached target iteration/insertion remains monotonic within each sorted source group and resets if an unexpected rebuild occurs. The tests cover forward/reverse links, duplicates, allocation rollback, ceiling fallback, grouping sets, stale topology, single/multi-CN ownership, and race/reuse paths. No correctness, leak, hang, or material regression blocker found.
What changed
Fixes #27672.
This change removes the serial high-NDV exact-state bottleneck exposed by ClickBench Q10 while preserving exact
COUNT(DISTINCT)semantics.COUNT(DISTINCT)argument state, not only the small final group hash table. For a large exact state with enough logical groups, rows are partitioned by the group key before final aggregation, so each logical group has one owner and no exact-state MergeGroup is required.max_dop=1reuses the ordinary non-shuffle topology; multi-CN DOP 1 and reused shuffles retain their parallel layouts.The independent cached-Inserter correctness fix and batch-fill preflight optimization are owned by #27688 and are already in
main.Root cause
The single-aggregate DISTINCT rewrite does not apply to Q10 because the node also contains
SUM,COUNT, andAVG. ExactCOUNT(DISTINCT)states therefore remain inside each local Group operator. The planner costed shuffle from roughly 100 finalRegionIDgroups and ignored the roughly one-million retainedUserIDvalues, so many partial ordered states converged on a serial MergeGroup. Fixed-size arena growth then repeatedly rebuilt historical state, while preflight and publication restarted ordered searches.Two boundary regressions found during review are also closed:
Design and boundaries
DuckDB uses a separate radix-partitioned table keyed by group keys plus DISTINCT arguments, then finalizes those unique tuples into the regular aggregate table in parallel. This PR adopts the same partition-before-finalize principle through MatrixOne's existing shuffle-group topology; it does not introduce a second aggregate framework.
The planner rule is deliberately bounded:
COUNT(DISTINCT)contributes exact-state cost;Extreme single-group or strongly skewed distributions cannot be solved by group-key shuffle and remain on the optimized executor merge path. A full independent radix DISTINCT pipeline could cover those shapes, but needs a separate design-reviewed change and better conditional/top-frequency statistics.
Performance and memory
BenchmarkCountDistinctSavedArgumentMergemodels 1,000,000 unique values, 16 partial states, and 100 groups on the same Apple M4 host:The allocator ceiling is checked only when an arena actually needs relocation, not per row or per key. Successful growth still uses the largest admissible geometric capacity; the smaller retry remains limited to classified account/pool capacity pressure.
Validation
pkg/common/mpool,pkg/common/arenaskl,pkg/sql/colexec/aggexec,pkg/sql/colexec/group,pkg/sql/plan, andpkg/sql/compile;pkg/common/arenaskl,pkg/sql/colexec/aggexec,pkg/sql/colexec/group, andpkg/sql/plan;go veton the affected/consumer packages with the repository CGo environment;git diff --check.The tests cover forward/reverse arena links, values, duplicate rejection, post-growth insertion, poisoned/overlapping arenas, transactional allocation failure, account pressure, allocator ceiling, filtered NDV, insufficient logical owners, low state/input ratio, invalid statistics, stale single-stage shuffle state, single-owner single/multiple inputs, ordered aggregates, and multi-CN DOP 1. No sleeps, large-data UTs, or weakened assertions were added.