Keep the part sorted when TTL GROUP BY ... SET rewrites a sorting key column by alexey-milovidov · Pull Request #119863 · ClickHouse/ClickHouse

alexey-milovidov · 2026-09-13T22:35:48Z

TTL ... GROUP BY ... SET emits the aggregated rows at the position of their group in the merged stream, but a SET expression may assign arbitrary values to a column of the sorting key (for example ORDER BY (k, toStartOfDay(ts)) with SET k = max(v)), so an aggregated row could be less than the previously emitted one. The merge wrote such a part as is: a debug build threw the logical error Sort order of blocks violated in CheckSortedTransform, and a release build silently produced a part whose primary index does not match the data.

Now TTLAggregationAlgorithm tracks the sorting key of the last emitted row. If the next row violates the order, its sorting key is replaced with the sorting key of the previous row, so the output stays sorted and the primary index stays consistent with the data. This is a simpler alternative to #108550, which re-sorts the output of the TTL step.

Closes: #108514
Related: #108550

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):

Fix the order of rows in a data part written by a merge with TTL ... GROUP BY ... SET when the SET clause assigns a new value to a column of the sorting key. Previously such a merge could produce a part whose primary index does not match the data (a logical error Sort order of blocks violated in debug builds). Now the sorting key of an out-of-order row is replaced with the sorting key of the previous row. Closes #108514.

Workflow [PR]
Sync PR [sync-upstream/pr/119863]

… column `TTL ... GROUP BY ... SET` emits the aggregated rows at the position of their group in the merged stream, but a `SET` expression may assign arbitrary values to a column of the sorting key, so an aggregated row could be less than the previously emitted one. The merge wrote such a part as is: a debug build threw `Sort order of blocks violated` in `CheckSortedTransform`, and a release build produced a part whose primary index does not match the data. Now `TTLAggregationAlgorithm` tracks the sorting key of the last emitted row. If the next row violates the order, its sorting key is replaced with the key of the previous row, so the output stays sorted and the primary index stays consistent with the data. Closes: #108514 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

… on the number of merges Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

clickhouse-gh · 2026-09-13T22:36:50Z

Workflow [PR], commit [aa22ea9]

Summary: ❌

job_name	test_name	status	info
AST fuzzer (amd_debug, targeted, old_compatibility)		FAIL
	Logical error: Sort order of blocks violated for column number A, left: B, right: C. Chunk D, rows read E.F (STID: 3413-350b)	FAIL	cidb
Stateless tests (amd_debug, flaky check)		FAIL
	Server liveness check failed	FAIL	cidb
	Logical error: Sort order of blocks violated for column number A, left: B, right: C. Chunk D, rows read E.F (STID: 3413-350b)	FAIL	cidb
Stateless tests (amd_debug, parallel)		FAIL
	Server liveness check failed	FAIL	cidb
	Logical error: Sort order of blocks violated for column number A, left: B, right: C. Chunk D, rows read E.F (STID: 3413-350b)	FAIL	cidb
Finish Workflow		FAIL
	python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py	FAIL
Fast test (arm_darwin)		DROPPED
Build (amd_darwin)		DROPPED
Build (arm_darwin)		DROPPED
Build (arm_v80compat)		DROPPED
Build (amd_freebsd)		DROPPED
Build (ppc64le)		DROPPED

AI Review

Summary

This PR tries to repair TTL ... GROUP BY ... SET sort-order corruption by clamping an out-of-order row's sorting key to the previous emitted key instead of re-sorting. The current implementation still leaves expression-based ORDER BY keys inconsistent with the stored row data, and it does not cover the MATERIALIZE TTL mutation path when the sorting key is longer than the primary key, so it does not close the original correctness bug yet.

Findings

❌ Blockers

[src/Processors/TTL/TTLAggregationAlgorithm.cpp:352-404] restoreSortOrder operates on the precomputed sorting-key helper columns already present in the block. For expression keys such as toStartOfDay(ts), finalizeAggregates leaves those helper columns at their pre-SET values, and this code copies the stale values into the repaired stream. MergeTreeDataPartWriter then rebuilds the primary index from those helper columns while the persisted row keeps the new base-column values, so expression-based keys can still leave the part/index contract broken. Suggested fix: recompute sorting-key expressions from the post-SET row before repairing or re-sorting the output.
[src/Processors/TTL/TTLAggregationAlgorithm.cpp:94-96] The mutation path still checks only a prefix of the sorting key. MutateTask materializes primary-key and skip-index expressions before TTLTransform, not the full sorting key, so a table like PRIMARY KEY k ORDER BY (k, toStartOfDay(ts)) reaches this break with only k available. restoreSortOrder then misses reorderings inside one k, so ALTER TABLE ... MATERIALIZE TTL can still write an invalid part. Suggested fix: materialize the full sorting key on mutations too, or reject this shape instead of silently checking only the prefix.

Tests

The new stateless test checks query output after ORDER BY ALL, so it does not validate the hidden sorting-key expression columns that feed the primary index, and it does not exercise the MATERIALIZE TTL path. A focused test should assert the physical sort-key behavior for an expression key and for ALTER TABLE ... MATERIALIZE TTL.

Final Verdict

Changes requested.

alexey-milovidov

LGTM

clickhouse-gh · 2026-09-13T23:01:46Z

+
+    Columns sort_key;
+    for (const auto & elem : sort_description)
+        sort_key.push_back(block.getByName(elem.column_name).column->convertToFullColumnIfSparse());


MergeTask materializes the ORDER BY expression columns before TTL, so here we are comparing and rewriting those helper columns, not the sort key that will be recomputed from the stored row later. In the ORDER BY (k, toStartOfDay(ts)) shape from the issue, finalizeAggregates keeps toStartOfDay(ts) at its pre-SET value even after ts is reassigned, and this code just copies that stale helper value forward. MergeTreeDataPartWriter then rebuilds the primary index from the helper columns while the persisted row keeps the new ts, so expression keys can still leave the part/index contract broken. I think the sort-key expressions need to be recomputed from the post-SET row before any order repair happens.

clickhouse-gh · 2026-09-13T23:01:46Z

+    for (size_t i = 0; i < sorting_key.column_names.size(); ++i)
+    {
+        /// The header of a mutation may lack the sorting key expressions that are not in the primary key.
+        if (!header.has(sorting_key.column_names[i]))


This drops the fix on the MATERIALIZE TTL path whenever the sorting key is longer than the primary key. MutateTask only materializes getPrimaryKeyAndSkipIndicesExpression() before TTLTransform, so for a table like PRIMARY KEY k ORDER BY (k, toStartOfDay(ts)) the header has k but not toStartOfDay(ts). Breaking here means restoreSortOrder silently checks only the k prefix, so a SET ts = ... that reorders rows inside one k still writes an invalid part during ALTER TABLE ... MATERIALIZE TTL. If we cannot build the full sorting key on mutations, this shape needs to be rejected instead of partially checked.

clickhouse-gh · 2026-09-14T00:20:46Z

Build profile diff (arm_release)

Comparing aa22ea9de with master 0c3b387da (stripped binary size, per-symbol sizes and ThinLTO time; object sizes against the warmup build of 9fcf3dbb8; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Object file sizes

4 object files changed (+23.09 KiB total), 0 added.

Object file	Master	PR	Δ
`src/CMakeFiles/dbms.dir/Processors/TTL/TTLAggregationAlgorithm.cpp.o`	228.27 KiB	248.73 KiB	+20.45 KiB (+8.96%)

716 more object files are built by the master warmup baseline only (it builds every object-file target, a pull request build only clickhouse-bundle) and not compared.

Compile time of recompiled translation units

8 translation units recompiled, 25 s compile time in total, 8 of them have a recent master baseline.

Job report

alexey-milovidov and others added 2 commits September 13, 2026 22:18

Insert all rows of the test in one part so the result does not depend…

aa22ea9

… on the number of merges Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

clickhouse-gh Bot added the pr-bugfix Pull request with bugfix, not backported by default label Sep 13, 2026

alexey-milovidov commented Sep 13, 2026

View reviewed changes

alexey-milovidov self-assigned this Sep 13, 2026

clickhouse-gh Bot reviewed Sep 13, 2026

View reviewed changes

clickhouse-gh Bot added the comp-ttl TTL rules and TTL merges (moving/recompressing/rolling-up/deleting parts). label Sep 13, 2026

groeneai mentioned this pull request Sep 13, 2026

Fix sort order violation for TTL GROUP BY with SET on a sorting key column #108550

Open

groeneai mentioned this pull request Sep 14, 2026

Fix TTL GROUP BY for columns with nested LowCardinality types #119879

Open

Navigation Menu

Uh oh!

Keep the part sorted when TTL GROUP BY ... SET rewrites a sorting key column - #119863

Keep the part sorted when TTL GROUP BY ... SET rewrites a sorting key column#119863
alexey-milovidov wants to merge 2 commits into
masterfrom
fix-ttl-group-by-set-sort-order

alexey-milovidov commented Sep 13, 2026 •

edited by clickhouse-gh Bot

Loading

Uh oh!

clickhouse-gh Bot commented Sep 13, 2026 •

edited

Loading

Uh oh!

alexey-milovidov left a comment

Uh oh!

clickhouse-gh Bot Sep 13, 2026

Uh oh!

clickhouse-gh Bot Sep 13, 2026

Uh oh!

clickhouse-gh Bot commented Sep 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

alexey-milovidov commented Sep 13, 2026 • edited by clickhouse-gh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

clickhouse-gh Bot commented Sep 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Tests

Final Verdict

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot commented Sep 14, 2026

Build profile diff (arm_release)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

alexey-milovidov commented Sep 13, 2026 •

edited by clickhouse-gh Bot

Loading

clickhouse-gh Bot commented Sep 13, 2026 •

edited

Loading