Skip to content

Fix quadratic aggregation in order with the prealloc-serialized method#110159

Merged
alexey-milovidov merged 4 commits into
masterfrom
fix-agg-in-order-prealloc-quadratic
Jul 13, 2026
Merged

Fix quadratic aggregation in order with the prealloc-serialized method#110159
alexey-milovidov merged 4 commits into
masterfrom
fix-agg-in-order-prealloc-quadratic

Conversation

@alexey-milovidov

@alexey-milovidov alexey-milovidov commented Jul 12, 2026

Copy link
Copy Markdown
Member

Aggregation in order (AggregatingInOrderTransform) with a multi-column numbers/strings GROUP BY key, where the data is sorted by a proper prefix of that key, was quadratic in the block size and did not respect max_execution_time. It surfaced as a "Hung check failed, possible deadlock found" in the server-side AST fuzzer.

When the sort order is only a prefix of the GROUP BY key, consume splits the block into runs of equal prefix values and aggregates each run with a hash table over the full key. A multi-column numbers/strings key selects the prealloc_serialized method, whose state serializes all of the block's keys up front on construction. Because a fresh state is built for every run (once per row in the worst case, when the prefix is highly selective), a single block became O(number_of_runs * block_size). And since one consume call processes a whole block without yielding, the query also ignored max_execution_time — which is what tripped the hung check.

The fix makes aggregation in order use the plain serialized method, whose construction is O(1) and which serializes keys lazily per row, restoring linear time. The prealloc_serialized method is kept for whole-block aggregation, where it is a win. The serialized keys are byte-identical between the two methods, so merging across pipeline stages is unaffected.

Reproducer (before the fix this runs for many minutes; after it is instant):

CREATE TABLE t (s String, n UInt64) ENGINE = MergeTree ORDER BY s;
INSERT INTO t SELECT toString(number), number FROM numbers(300000);
SELECT s, n FROM t GROUP BY s, n
SETTINGS optimize_aggregation_in_order = 1, optimize_read_in_order = 1, max_block_size = 16384;

Verified locally: the reproducer above (N = 40000, single block) dropped from ~76 s to ~0.1 s under an ASan build, scales linearly, and produces results identical to regular aggregation across single/multi-key, nullable, prefix-sorted and full-sorted shapes; existing 02233_optimize_aggregation_in_order_prefix* and 01291_aggregation_in_order tests still pass.

No existing tracking issue was found for this failure.

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=81f15640fa009593bf323fc5b4259ee834143073&name_0=MasterCI&name_1=Stress%20test%20%28amd_tsan%29

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

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

Fixed a quadratic-time blowup (and unresponsiveness to max_execution_time) in aggregation in order when grouping by multiple keys whose sort order is only a prefix of the grouping key.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

Version info

  • Merged into: 26.7.1.866 (included in 26.7 and later)

Aggregation in order (`AggregatingInOrderTransform`) with a multi-column
numbers/strings GROUP BY key, where the data is sorted by a proper prefix of
the key, was quadratic in the block size and did not respect
`max_execution_time`. It showed up as "Hung check failed, possible deadlock
found" in the server-side AST fuzzer (`Stress test (amd_tsan)`).

When the sort order is only a prefix of the GROUP BY key, `consume` splits the
block into runs of equal prefix values and aggregates each run with a hash
table over the full key. A multi-column numbers/strings key selects the
`prealloc_serialized` method, whose state serializes all of the block's keys up
front on construction. Because a fresh state is built for every run (once per
row in the worst case, when the prefix is highly selective), a single block
became O(number_of_runs * block_size). And since one `consume` call processes a
whole block without yielding, the query also ignored `max_execution_time`,
which is what tripped the hung check.

Fix: use the plain `serialized` method for aggregation in order, whose
construction is O(1) and which serializes keys lazily per row, restoring linear
time. The `prealloc_serialized` method is kept for whole-block aggregation,
where it is a win. Results are byte-identical between the two methods, so
merging across pipeline stages is unaffected.

CI report:
https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=81f15640fa009593bf323fc5b4259ee834143073&name_0=MasterCI&name_1=Stress%20test%20%28amd_tsan%29

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [c137b39]

Summary:


AI Review

Summary

This PR narrows the prealloc_serialized fallback to the per-run aggregation-in-order path and adds focused regression coverage for the single-stream, multi-stream, and nullable-key cases. I did not find any remaining correctness or contract regressions on the current PR head: the earlier merge-path and nullable-coverage review points are addressed, the new method_chosen_for_in_order split is consistent with the unchanged whole-block merge path, and the current PR CI is green.

Final Verdict

✅ No new findings on the current PR head.

@clickhouse-gh clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Jul 12, 2026
Comment thread src/Interpreters/Aggregator.cpp
…h only

Addressing review (`clickhouse-gh` / AI Review "Request changes"): the previous
commit downgraded `method_chosen` for the whole `Aggregator` instance. In the
multi-stream in-order pipeline the same `Aggregator` is reused by
`MergingAggregatedBucketTransform`, and `Aggregator::mergeBlocks` seeds its
whole-block merge method from `method_chosen`. So for `GROUP BY s, n` with
`max_threads > 1` the final whole-block merge was also downgraded from
`prealloc_serialized_hash64` to `serialized_hash64`, contrary to the PR's stated
intent of keeping `prealloc_serialized` for whole-block aggregation.

Introduce a dedicated `method_chosen_for_in_order`, used only by the per-run
`executeOnBlockSmall` / `mergeOnBlockSmall` path (called only from
`AggregatingInOrderTransform`). It equals `method_chosen` except that the
`prealloc_serialized` variants are replaced by their plain `serialized`
counterparts. All whole-block paths, including `mergeBlocks`, keep
`method_chosen`, so `prealloc_serialized` stays enabled for the merge stage.
The `serialized` and `prealloc_serialized` methods produce byte-identical keys
and share the same `HashMethodSerializedContext`, so mixing them across pipeline
stages is safe.

Extend `04537_aggregation_in_order_serialized_keys` with a multi-stream
(`max_threads > 1`) case that reads several parts in order to exercise the
`MergingAggregatedBucketTransform` merge path, checks the result is identical to
regular aggregation, and still respects `max_execution_time`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alexey-milovidov added a commit that referenced this pull request Jul 12, 2026
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/Interpreters/Aggregator.cpp
…ialized fallback

The AI review (`clickhouse-gh`) noted that the earlier fix also touches the
`nullable_prealloc_serialized -> nullable_serialized` path in the `Aggregator`
constructor (`src/Interpreters/Aggregator.cpp`), but the regression test only
exercised the non-nullable `prealloc_serialized -> serialized` leg, so nothing
would fail if the nullable branch were removed.

Extend `04537_aggregation_in_order_serialized_keys` with a `Nullable(String),
Nullable(UInt64)` variant. A string plus a number is not all-fixed-contiguous,
so this multi-column nullable key selects `nullable_prealloc_serialized`, which
serializes keys through a different, null-map-aware path in `HashMethodSerialized`.
The data carries actual `NULL` values in both keys (disjoint across columns, so
every `(s, n)` pair stays distinct), exercising the null-map serialization. The
new case checks both the `max_execution_time` guard (quadratic blowup on the
per-run in-order path before the fix) and result equality against regular
aggregation.

Verified against a local Release build: with the fix the query returns in ~0.75s
and matches regular aggregation; with the nullable branch removed it exceeds
`max_execution_time` (`TIMEOUT_EXCEEDED` after ~20s), so the test now fails if
that line is dropped.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alexey-milovidov added a commit that referenced this pull request Jul 12, 2026
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The regression test used `max_execution_time = 20` (and `60` for the
multi-stream case) as a guard against the quadratic blowup. That limit
was too tight: under the `amd_asan_ubsan` flaky check the fixed, linear
path still takes around 20 seconds under parallel load, so it crossed the
20-second limit on several runs and reported `TIMEOUT_EXCEEDED`.

Measured runtimes confirm the limit was the only problem, not the fix:
the linear in-order path is ~0.13s (non-nullable) / ~0.26s (nullable) on a
release build and block-size independent, while the quadratic path is ~34s
on release and hundreds to thousands of seconds under sanitizers. The two
runtimes are orders of magnitude apart, so a generous limit separates them
cleanly on every build type.

Raise `max_execution_time` to 120 for all three guarded queries. This
clears the linear path with a wide margin on the sanitizer builds (where
it flaked) while staying far below the quadratic runtime on the
instrumented builds that run this suite, so the regression is still caught
without any risk of a false timeout on the fixed code. Also clarify the
comments to describe the guard's intent and note that the multi-stream
case's wall-clock is only a loose backstop (its value is the correctness
check).

Flaky check report:
https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110159&sha=08f8a567b642192488150ac9051193462fea6137&name_0=PR&name_1=Stateless%20tests%20%28amd_asan_ubsan%2C%20flaky%20check%29

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

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: 19/20 (95.00%) · Uncovered code

Full report · Diff report

alexey-milovidov added a commit that referenced this pull request Jul 13, 2026
The `amd_debug` flaky check timed out `04512_aggregation_in_order_serialized_keys`
at ~57s against the fixed (linear) in-order aggregation, while the query-level
`max_execution_time` guard was pinned to 20s.

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 release build
the linear path takes a fraction of a second while the quadratic one takes tens of
seconds; on a debug or sanitizer build the linear path can still take tens of
seconds under the parallel load of the flaky check (which is why the 20-second
limit flaked), while the quadratic regression runs for many minutes. Raising the
limit to 120s (matching the sibling test in #110159) clears the linear path with a
wide margin on every build and is still far below the quadratic runtime, so it
catches a regression without any risk of a false timeout on the fixed code.

Verified on the local `RelWithDebInfo` build that the fixed in-order path is
linear: N=200000 -> 0.04s, N=800000 -> 0.13s, N=1600000 -> 0.19s (quadratic would
be ~16x per 4x of N); the full test still produces the expected `1/1/1`.

CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110161&sha=b4623f4e6827d7339aefa49cb9f457033152670b&name_0=PR&name_1=Stateless%20tests%20%28amd_debug%2C%20flaky%20check%29

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@alexey-milovidov alexey-milovidov left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's perfect.

@alexey-milovidov alexey-milovidov self-assigned this Jul 13, 2026
@alexey-milovidov
alexey-milovidov added this pull request to the merge queue Jul 13, 2026
Merged via the queue into master with commit 961ded3 Jul 13, 2026
176 checks passed
@alexey-milovidov
alexey-milovidov deleted the fix-agg-in-order-prealloc-quadratic branch July 13, 2026 14:56
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants