perf: linearize DISTINCT aggregate preflight deduplication - #27688
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? |
|
Deep review(base
后两种是正常可达的
建议:
正确性方面目前未发现问题:hash collision 有完整比较兜底,NULL、const、offset、多列、signed-zero 语义保持一致;表容量有界且无共享状态。精确 head 的 aggexec 包测试和 |
d67e2dc to
353ed46
Compare
|
Addressed the deep-review findings in
Normal tests, full race tests, vet, and diff-check pass locally after rebasing to current main. Full commands and benchmark numbers are in the updated PR body. |
LeftHandCold
left a comment
There was a problem hiding this comment.
Reviewed the exact head 353ed46. Checked the adaptive DISTINCT preflight paths, NULL/const/offset/multi-column/signed-zero semantics, bounded hash-table transition, allocation accounting, and cached Inserter equality fix. Also ran the focused arenaskl tests and diff-check. I found no reachable correctness or regression blocker. The remaining Ubuntu/SCA/BVT checks were still running at review time.
## What changed Fixes #27672. This change removes the serial high-NDV exact-state bottleneck exposed by ClickBench Q10 while preserving exact `COUNT(DISTINCT)` semantics. - Teach group-shuffle planning to account for mergeable `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. - Estimate the DISTINCT state and effective group owners after selection. Low state/input ratios, too few surviving owners, unknown statistics, and unsupported keys stay on the existing topology. - Explicitly disable shuffle for aggregate nodes that contain a non-mergeable DISTINCT state and are forced to one CN, including stale/reused shuffle state. - At compile time, skip a normal group shuffle when the scheduled topology exposes only one physical aggregate owner. Single-CN `max_dop=1` reuses the ordinary non-shuffle topology; multi-CN DOP 1 and reused shuffles retain their parallel layouts. - Relocate arena-backed skiplists during preflight capacity growth instead of rebuilding every retained node, and grow arenas geometrically. - Clamp speculative geometric growth to MPool's real single-allocation limit before allocation. If account or pool capacity rejects the preferred arena while the old arena is live, retry the smaller former chunked-growth capacity. Terminal ownership/invariant errors are not retried. - Use one monotonic target iterator and one cached Inserter for sorted state merges instead of restarting target searches for every candidate. 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`, and `AVG`. Exact `COUNT(DISTINCT)` states therefore remain inside each local Group operator. The planner costed shuffle from roughly 100 final `RegionID` groups and ignored the roughly one-million retained `UserID` values, 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: - a geometric request could exceed MPool's roughly 2 GiB single-allocation ceiling and fail around a 1 GiB current arena even though the linear fallback still fit; - a single-CN, single-owner plan could still compile a one-bucket Shuffle plus Dispatch, adding per-row hashing and pipeline overhead without exposing parallel aggregate ownership. ## 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: - only mergeable `COUNT(DISTINCT)` contributes exact-state cost; - table/expression NDV is capped by input rows and conservatively adjusted by available selection statistics; - low exact-state/input ratios do not trigger a full-row shuffle; - fewer than 64 estimated logical owners do not trigger; - existing supported integer/string shuffle keys are required; - non-mergeable DISTINCT aggregates retain the single-stage contract and cannot retain stale shuffle state; - a normal shuffle must expose at least two physical aggregate owners after DOP and CN placement are known. 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 `BenchmarkCountDistinctSavedArgumentMerge` models 1,000,000 unique values, 16 partial states, and 100 groups on the same Apple M4 host: - before this PR: 1.632 s/op; - latest head, 5 runs: 146.6-153.5 ms/op, median 148.1 ms/op; - improvement: about 10.6x-11.1x; - latest allocation sample: 21-23 KiB/op and 4,813-4,819 allocs/op. 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 - full UT: `pkg/common/mpool`, `pkg/common/arenaskl`, `pkg/sql/colexec/aggexec`, `pkg/sql/colexec/group`, `pkg/sql/plan`, and `pkg/sql/compile`; - full race: `pkg/common/arenaskl`, `pkg/sql/colexec/aggexec`, `pkg/sql/colexec/group`, and `pkg/sql/plan`; - 100 race repetitions of arena relocation, account-pressure and allocator-ceiling fallback, and each new single-owner/multi-CN compiler topology counterexample; - `go vet` on 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.
What this changes
Make saved-argument DISTINCT preflight adaptive instead of forcing either quadratic scans or hashing every row:
(target group, DISTINCT arguments)representatives and use exact equality only;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.Insertercould 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 addsInserter.AddWithPlanso 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):All focused cases remain at 0 B/op and 0 allocs/op. The existing 65,536-row
BenchmarkCountDistinctSavedArgumentsremains about 48-49 ms/op.Stack frames from the compiled test binary:
Regression coverage
Exact package coverage comparison (
pkg/common/arenaskl+pkg/sql/colexec/aggexec):Validation
go vetfor both packages;git diff --check;origin/main(e8332cb8cc).