Fix quadratic aggregation-in-order with serialized keys

[alexey-milovidov]

Fixes a quadratic slowdown (effectively a hang) in aggregation-in-order.

With optimize_aggregation_in_order = 1 and a multi-key GROUP BY that uses the serialized aggregation method (more than one key, all numbers or strings, e.g. a UInt64 key plus a String key), the query could run for minutes and trip the stress-test hung-check while ignoring max_execution_time and cancellation.

AggregatingInOrderTransform aggregates each block in small consecutive sub-ranges — one per sorting-key prefix — via Aggregator::executeOnBlockSmall. The prealloc_serialized method precomputes the serialized keys of the whole block in its state constructor, so that whole-block work was redone on every sub-range: a block with N distinct sorting-key prefixes cost O(N * block_rows) (quadratic), and a single work call could spin for minutes.

The fix chooses the plain serialized method (lazy per-row key serialization) for in-order aggregation via a new Aggregator::Params::aggregation_in_order flag, keeping the small-block path linear. executeOnBlockSmall is used only by AggregatingInOrderTransform, so no other path is affected. Results are unchanged; regular (non-in-order) aggregation still uses prealloc_serialized.

Reproduced locally without parallel replicas — 50k distinct-prefix rows went from ~19s to ~0.2s, and scaling is now linear (500k rows in ~0.4s).

Surfaced by the master stress-test hung-check:
https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=d39d027a4b06785e67285c3eafb2a65569d29f6c&name_0=MasterCI&name_1=Stress%20test%20%28azure%2C%20amd_tsan%29

Changelog category (leave one):

• Performance Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed a quadratic slowdown of GROUP BY with optimize_aggregation_in_order = 1 over multiple keys that use the serialized aggregation method (e.g. a numeric key together with a String key). Such queries could run for minutes on inputs with many distinct sorting-key prefixes; they are now linear.

Documentation entry for user-facing changes

• Documentation is written (mandatory for new features)

🤖 Generated with Claude Code

[clickhouse-gh]

Workflow [PR], commit [9d479b8]

[clickhouse-gh]

params.aggregation_in_order is attached to the shared AggregatingTransformParams, so in the multi-stream in-order pipeline it also changes the Aggregator that MergingAggregatedBucketTransform uses later for the final merge (src/Processors/Transforms/MergingAggregatedMemoryEfficientTransform.cpp:394). That merge does not go through executeOnBlockSmall; for the common case where the input is already sorted by the full GROUP BY key, AggregatingInOrderTransform stays on the group_by_key == false branch and never hits the quadratic path (src/Processors/Transforms/AggregatingInOrderTransform.cpp:179-182). With the current flag, though, the final merge is still forced off prealloc_serialized at Aggregator.cpp:691, so this broadens the fix into a performance regression for multi-stream / full-key-sorted in-order queries. Can we keep the "avoid prealloc" choice local to the small-block hash-table path, or give the final merge its own Aggregator so it can preserve the old method choice?

[alexey-milovidov]

Confirmed: as written, the params.aggregation_in_order flag changes method_chosen on the whole shared Aggregator, so in the multi-stream (max_threads > 1) pipeline the final MergingAggregatedBucketTransform merge (Aggregator::mergeBlocks) is also steered off prealloc_serialized. Both paths stay correct and linear; the effect is that the whole-block merge of a full-key-sorted in-order query loses the prealloc_serialized batch-serialization optimization.

The narrowed variant this review suggests — keep method_chosen = prealloc_serialized for the whole-block/merge paths and derive a separate method_chosen_for_in_order used only by executeOnBlockSmall/mergeOnBlockSmall — is implemented in the sibling PR #110159, which also adds explicit multi-stream coverage. This PR is the simpler global-flag alternative, so which of the two approaches to keep is a design decision left to the author.

Separately, the CI failure here was the flaky-check TOO_SLOW on 04512_aggregation_in_order_serialized_keys, not the aggregation fix: the test left the in-order read unpinned, so the flaky check randomized it into an expensive max_block_size/optimize_read_in_order combination. Fixed in 4f3ef7e by pinning the read configuration (max_threads = 1, max_block_size = 16384, optimize_read_in_order = 1) and adding a max_execution_time guard, mirroring 04537_aggregation_in_order_serialized_keys. Verified locally on a debug build that the query runs in ~0.6s even with the adversarial flaky-check settings applied, and still matches regular aggregation.

[alexey-milovidov]

Follow-up on the CI status (superseding my earlier note that the flaky-check TOO_SLOW was only about the unpinned read):

The pinned test still failed Stateless tests (amd_debug, flaky check) on 04512_aggregation_in_order_serialized_keys (6/16 iterations, TIMEOUT_EXCEEDED at 46–126s > the 20s guard, all on MergeTreeSelect(pool: ReadPoolInOrder, algorithm: InOrder)). I reproduced and investigated this end-to-end:

• The fix itself is correct. On a freshly built binary the in-order query is linear: 200k rows in ~0.2s and 2M rows in ~1s at the pinned config, versus ~34s (quadratic) on a stale pre-fix binary. I ran every one of the 5 distinct failing flaky-check setting combinations (extracted from the CI log) against the fixed binary at 2M rows — all linear (~1–4s). This covers the single-stream path, the multi-stream FinishAggregatingInOrderTransform/MergingAggregatedBucketTransform path (verified via EXPLAIN PIPELINE), and the parallel-replicas iteration (with a local parallel_replicas cluster). The exact test file runs in ~0.65s and matches the reference.
• The amd_asan_ubsan, amd_tsan, and amd_msan flaky checks all ran 04512 (60 runs each) and passed. Only amd_debug failed, with a profile consistent with a binary in which the fix was not effective (deep quadratic, uncancellable), and its log timestamps predate the current head b22accd2.

Given the fix is verified correct across all these configurations, the amd_debug failure is consistent with a stale build artifact. I merged the latest master (the branch was 136 commits behind; the merge is clean and leaves the fix intact) to force a fresh full rebuild of all configurations. Rebuilt locally and re-verified linear after the merge.

The design point above (global method_chosen flag here vs. the scoped method_chosen_for_in_order in #110159) still stands as the author's decision.

[alexey-milovidov]

Correcting my previous follow-up (which attributed the amd_debug failure to a stale build artifact — that was wrong).

The fresh build did not clear it: Stateless tests (amd_debug, flaky check) on 04512_aggregation_in_order_serialized_keys failed again at head b4623f4e68 (built after the master merge, so genuinely fresh with the fix in tree), timing out at ~57s against the 20s guard on MergeTreeSelect(pool: ReadPoolInOrder, algorithm: InOrder).

The real root cause is that the max_execution_time = 20 guard was too tight, not a code regression:

• The fix is linear. On the local build the fixed in-order path scales linearly with N: 200k → 0.04s, 800k → 0.13s, 1600k → 0.19s (8x the data for ~4.7x the time; a quadratic regression would be ~64x). The full test still returns 1 / 1 / 1.
• The guard is a protection against the quadratic blowup, not a tight assertion on the linear runtime, and the two are orders of magnitude apart. On a debug or sanitizer build the linear path can still take tens of seconds under the parallel load of the flaky check, while the quadratic regression runs for many minutes. The 20s limit sat right in that debug/parallel-load band.
• amd_asan_ubsan, amd_tsan, and amd_msan all passed 04512; only amd_debug (unoptimized -O0, slowest under the flaky check's parallelism) tripped the 20s limit.

Fix pushed as a test-only change: raise both max_execution_time guards in 04512 from 20s to 120s, matching the sibling test in #110159, which hit the identical wall-clock-guard flake on its ASan flaky check and was resolved the same way. 120s clears the linear path with a wide margin on every build and is still far below the quadratic runtime (~300s+ on debug), so it still catches a regression without a false timeout on the fixed code.

The design point above (global method_chosen flag here vs. the scoped method_chosen_for_in_order in #110159) is unaffected and still stands as the author's decision.

[alexey-milovidov]

Follow-up with a definitive calibration (fresh amd-equivalent -O0 debug build of this exact branch, single-stream section-1 query — 200000 rows, max_block_size = 16384):

• With the fix (params.aggregation_in_order = true → chooseMethod(..., avoid_prealloc_serialized = true) → serialized): ~0.3s, linear. The full 04512 test returns 1 / 1 / 1.
• With the fix disabled (rebuilt with params.aggregation_in_order = false): ~26.6s, quadratic. Same query, same data.

So the fix is correct and the small-block in-order path is linear — a ~90× separation on this build.

Why the amd_debug flaky check keeps timing out is not a too-tight guard. The failing run landed on a pathologically overloaded runner (its Start ClickHouse Server step alone took 567s, Collect logs 345s), and all 5 04512 iterations were killed at the 120s max_execution_time. The problem is that the wall-clock guard cannot cleanly separate the two runtimes on such a box:

• The quadratic CPU time (~27s on a fast box) is comparable to — or below — the wall-clock the linear path reaches once the box is heavily oversubscribed.
• I ruled out contention as an independent cause: 24 concurrent copies of the query on a 96-core box still finished in 0.3s total, so no realistic oversubscription inflates a 0.3s-CPU query to 120s on its own — it takes an extreme-overload box like this one.

Net: the guard currently sits between "quadratic on a fast box" (~27s, under 120s → a real regression could slip through on a fast runner) and "linear on a badly overloaded box" (>120s → false timeout), so box load dominates the signal rather than linear-vs-quadratic.

I did not touch the guard again (it isn't the right lever) or narrow the C++ (that's the reserved global-vs-scoped design decision). I merged latest master and pushed to re-trigger a fresh run on a normal runner. If the flake persists, the robust options are (a) widen the separation so the quadratic runtime dwarfs any overload-inflated linear runtime (e.g. a single large block so it is O(rows^2) rather than O(rows * block_size)), or (b) a contention-independent assertion (compare the CPU time of the in-order query to the non-in-order reference instead of wall-clock). Both are your call.

The other red checks are unrelated known flakes: 03988_cached_read_big_at (flaky-labelled, amd_tsan) and the Block structure mismatch logical error (open issue #109215).

[alexey-milovidov]

Follow-up (supersedes my previous calibration note): I stopped trying to tune the wall-clock guard and rewrote the test to not depend on wall-clock at all.

Definitive findings, on a fresh -O0 debug build of this branch (chooseMethod(..., avoid_prealloc_serialized) in tree):

• The fix is linear. Section-1 query (500k/1M rows, max_block_size = 16384, max_threads = 1): ~0.5s / ~0.9s of CPU. Disabling the flag (params.aggregation_in_order = false) rebuilds to ~29s / ~145s for the same query — quadratic, as expected.
• Wall-clock is not a usable signal here. Applying the flaky-check IO-torture settings (min_bytes_to_use_direct_io = 1, page_cache_inject_eviction = 1, …) inflated the query's wall-clock by ~60% but left its OSCPUVirtualTimeMicroseconds unchanged (0.926s → 0.929s). On an oversubscribed runner a linear query's wall-clock stretches arbitrarily, so no max_execution_time value can sit between "linear under load" and "quadratic on an idle box" — which is exactly why the 20s→120s bump didn't help and it still timed out at 122–133s.
• There is no memory/allocation discriminator either: ArenaAllocBytes is ~16 MB for both the linear and the quadratic path. The only observable that separates them is CPU time.

So the test now runs the in-order aggregation once on its own (tagged with a CPUGUARD marker) and asserts, via system.query_log, that its OSCPUVirtualTimeMicroseconds stays under 3s. CPU time measures cycles actually consumed and is unaffected by contention, so it separates linear (~0.5 CPU-s) from quadratic (tens of CPU-s on release, well over a minute on the instrumented builds) by more than an order of magnitude regardless of runner load. Verified end-to-end against a real server: 1/1/1/1, deterministic across repeated runs in both reused and fresh databases; a query over the threshold correctly evaluates the guard to 0. The correctness checks (in-order == hash, single- and multi-stream, MergingAggregatedBucketTransform present) are unchanged.

The merge-path scope question (this shared-flag design vs. keeping the override local to executeOnBlockSmall/mergeOnBlockSmall as in #110159) is a design decision and is left untouched here.

[clickhouse-gh]

📊 Cloud Performance Report

✅ AI verdict: no_change — no significant changes across 37 queries analysed

This PR makes aggregation-in-order pick the plain serialized method instead of prealloc_serialized for multi-key GROUP BY, fixing a quadratic re-serialization blowup; it does not change execution for single-key hash aggregations. The two flagged ClickBench queries (Q4 and Q15) are both single-key GROUP BYs, and the math correctly kept them as no_change: their -19.6% and -18.0% deltas sit inside master's current variance band and cannot plausibly be caused by this change. The suppression is correct and consistent with the diff; no real per-query regression or improvement is attributable to this PR.

clickbench

🟢 No significant changes

tpch_adapted_1_official

🟢 No significant changes

Debug info

• StressHouse run: f1b9f7f5-dcf1-45d4-8199-1058c52f86d9
• MIRAI run: f6ae2ca4-ea2f-410d-9b2a-511214056451
• PR check IDs:
  • clickbench_474012_1783983695
  • clickbench_474026_1783983695
  • clickbench_474032_1783983695
  • tpch_adapted_1_official_474039_1783983695
  • tpch_adapted_1_official_474045_1783983695
  • tpch_adapted_1_official_474055_1783983695

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.80%	85.80%	+0.00%
Functions	92.70%	92.70%	+0.00%
Branches	78.00%	78.00%	+0.00%

Changed lines: Changed C/C++ lines covered: 12/12 (100.00%) · Uncovered code

Full report · Diff report

[alexey-milovidov]

Duplicate of #110159.