All committers have signed the CLA.

Workflow [PR], commit [fe61ffb]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds opt-in recursive Tuple leaf aggregation for SummingMergeTree, AggregatingMergeTree, and CoalescingMergeTree, with docs, schema validation, and stateless coverage. The remaining blocker is that the AggregatingMergeTree merge path does not protect tuple leaves used by PARTITION BY, so the new aggregation can mutate partition-key values during FINAL, OPTIMIZE, or merge-on-insert.

Findings

❌ Blockers

• [src/Processors/Merges/Algorithms/AggregatingSortedAlgorithm.cpp:24] AggregatingSortedAlgorithm decides aggregate vs copied tuple leaves using only the sorting key. The SummingMergeTree and CoalescingMergeTree paths pass partition+sorting required columns, but the AggregatingMergeTree call sites do not, so an opt-in table with metrics.part SimpleAggregateFunction(sum, UInt64) inside a Tuple, PARTITION BY metrics.part, and ORDER BY id will aggregate metrics.part for rows in the same partition. That can leave a part named/minmaxed for partition 10 containing a row whose partition leaf became 20, breaking partition metadata and pruning assumptions. Pass partition+sorting required columns through ReadFromMergeTree, MergeTask, MergeTreeDataWriter, AggregatingSortedTransform, and AggregatingSortedAlgorithm, then exclude matching leaves or ancestors from aggregation.

Tests

• ⚠️ Add a focused AggregatingMergeTree regression with allow_tuple_element_aggregation = 1, a tuple SimpleAggregateFunction leaf in PARTITION BY, and duplicate ORDER BY keys, verifying both FINAL and OPTIMIZE TABLE ... FINAL preserve the partition leaf while aggregating non-key leaves.

Final Verdict

Status: ❌ Block

Minimum required action: fix the AggregatingMergeTree partition-key exclusion and add the regression above.

The Stress test (arm_msan) failure is fixed by #101239, which should be merged first. After it is merged, please update the branch to include the fix.

Thanks for working on this!

I was trying this query but it does not sum:

CREATE TABLE t (key UInt32, value Tuple(a UInt64, b UInt64))
ENGINE = SummingMergeTree(value) ORDER BY key
SETTINGS allow_tuple_element_aggregation = 1;

INSERT INTO t VALUES (1, (10, 20));
INSERT INTO t VALUES (1, (30, 40));
OPTIMIZE TABLE t FINAL;
SELECT * FROM t;

1       (10,20)

Thank you very much for pointing out this issue. The root cause was that, after the tuple was flattened, the resulting names could not be matched against the source column names recorded in column_names_to_sum, leading to an inability to identify them. I am now resolving this issue by flatten column_names_to_sum alongside the block. Additionally, I have added corresponding test cases to ensure adequate coverage.

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 387/401 (96.51%) | Lost baseline coverage (was covered on master, now uncovered in this PR): 3 line(s) · Uncovered code

Full report · Diff report

Hi @JingYanchao @nihalzp — while reviewing this PR I found the following:

• Coverage gap: Flattened xxxMap inside a Tuple whose key array is in the sorting key is excluded from summation — break at SummingSortedAlgorithm.cpp:495 untested — Add test: Flattened xxxMap inside a Tuple whose key array is in the sorting key is excluded from summation — break at SummingSortedAlgorithm.cpp:495 untested #107193

Could you review the test PR and add the can be tested label if it looks good?
Happy to discuss — close anything that's wrong or already addressed.