Reduce memory consumption in aggregator

[scanhex12]

Changelog category (leave one):

• Improvement

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

Reduce memory consumption in aggregator

Version info

• Merged into: 26.7.1.473 (included in 26.7 and later)
• Backported to: 26.6.2.27

[clickhouse-gh]

Workflow [PR], commit [e4b7f58]

Summary: ❌

• Performance Comparison: Performance dashboard

job_name	test_name	status	info	comment
Stress test (amd_asan_ubsan)		FAIL
	Hung check failed, possible deadlock found	FAIL	cidb, issue
Stress test (arm_tsan)		FAIL
	Logical error: Block structure mismatch in A stream: different number of columns: (STID: 0993-308a)	FAIL	cidb, issue

---

AI Review

Summary

This PR tries to reduce memory usage while spilling aggregated data to temporary files by moving the converted columns out of AggregatedChunk before writing them. The ownership shuffle is not enough on its own, though: the spilled columns still stay alive in the local Block until the same point in the loop, so the claimed memory reduction is not established by the current implementation.

Findings

⚠️ Majors

• [src/Interpreters/Aggregator.cpp:1945] The PR promises lower memory usage during external aggregation, but detachColumns() here only transfers ownership from agg_chunk.chunk to block. Block::setColumns copies the same ColumnPtrs into block, NativeWriter::write consumes that Block synchronously, and block is still kept alive through update_max_sizes(block) below. That means the ColumnAggregateFunction objects and their arenas are released at the same point as before, so the spill path does not actually free memory earlier.
  Suggested fix: either destroy/clear block immediately after out->write(block) and compute the size stats before that, or use another approach that demonstrably shortens the lifetime of the spilled columns.

Tests

• ⚠️ This is an Improvement PR, but there is no focused before/after measurement showing reduced peak memory on the external aggregation spill path. A small repro that forces spilling and reports peak memory before and after the fix would close the gap.

Final Verdict

Status: ⚠️ Request changes

Minimum required actions:

1. Make the spill path actually release the written aggregate columns earlier, or explain with evidence why the current ownership change already reduces peak memory.
2. Add a focused before/after measurement for the external aggregation spill case.

[grantholly-clickhouse]

IIUC, we are now going to move the columns to the block so that they are not held onto until the very end by the source chunk.

[grantholly-clickhouse]

LGTM

[clickhouse-gh]

detachColumns() only moves ownership from agg_chunk.chunk into the local block; it does not shorten the lifetime of the spilled aggregate columns themselves. Block::setColumns still copies those ColumnPtrs into block, NativeWriter::write is synchronous, and block stays alive through update_max_sizes(block) below, so the ColumnAggregateFunction objects and their arenas are released at the same point as before. As written, this does not actually deliver the PR's promised memory reduction on the spill path. Please either drop block immediately after the write (or compute the stats before the write and clear it afterward) and attach a before/after measurement, or explain what other lifetime change is supposed to lower peak memory here.

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.50%	85.50%	+0.00%
Functions	92.60%	92.60%	+0.00%
Branches	77.70%	77.70%	+0.00%

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

Full report · Diff report