Skip to content

[feature](be) Add aggregate state finalizers and batch merging - #68312

Open
HappenLee wants to merge 6 commits into
apache:masterfrom
HappenLee:feature/agg-state-finalize
Open

HappenLee wants to merge 6 commits into
apache:masterfrom
HappenLee:feature/agg-state-finalize

Conversation

@HappenLee

@HappenLee HappenLee commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Problem Summary:

A query that has already computed one aggregate state per key still needs an aggregate operator to obtain its final values through <agg>_merge. This repeats aggregation for the finest grouping when states are also reused for coarser rollups.

Add the scalar <agg>_finalize(state) combinator. It returns one result for each input state, using the existing aggregate implementation and serialized-state representation. For example:

SELECT k, avg_finalize(s)
FROM (SELECT k, avg_combine(v) AS s FROM t GROUP BY k) partial;

The same implementation supports aggregates such as count, sum, min, max, and array_agg. FE validates that the state's canonical aggregate name matches the finalizer, derives its result type, and treats the function as scalar. BE handles the underlying serialized column type, skips outer NULL payloads, and releases temporary state after each row. Constant inputs use the ordinary scalar constant path. Existing empty-state semantics and serialized formats are unchanged.

MV rollup matching follows only direct MERGE/UNION state chains for the same aggregate and preserves the full value expressions of STATE/COMBINE. This prevents scalar finalizers nested in their arguments from making different outer aggregates or different value expressions appear equivalent, while retaining compatible state rollups.

Grouped *_union and *_merge now delegate to the existing vectorized deserialize-and-merge interfaces, including the selected path. A batch-local aligned Arena supplies generic temporary state storage; nested implementations manage construction/destruction, and the caller Arena retains destination allocations. This avoids shared mutable scratch buffers and per-row range dispatch. The Nullable selected fast path now reads each selected row independently so skipped destinations cannot shift the serialized input cursor. Single-place merging, FE plans and scalar finalization are unchanged by this follow-up. Q67 A/B performance has not been measured.

Release note

Add <aggregate>_finalize(AGG_STATE) scalar functions to retrieve each aggregate state's result without merging rows.

Check List (For Author)

  • Test
    • Regression test: test_agg_state_finalize on a fresh local ASAN BE + FE cluster; output generated by the standard runner, checked against direct original aggregates, and verified by a normal comparison run.
    • Regression test: agg_state_finalize_roll_up verifies MV rejection for differing outer aggregates/scalar arguments and selection for matching rollups, including negative values and NULLs. Generated output was checked independently; the normal comparison run of this suite and test_agg_state_finalize passed.
    • Unit Test: 51 FE tests passed across CombinatorRollupHandlerTest, CountTest, FinalizeCombinatorTest, StateCombinatorTest, CombineCombinatorTest, and FunctionRegistryTest. The 9 new rollup tests reproduced 6 failures before the fix and all pass after it. All 9 FunctionAggStateFinalizeTest cases passed under ASAN.
    • Manual test: compare grouped and empty AVG/COUNT/SUM/MIN/MAX, decimal AVG and ARRAY_AGG with the original aggregates; verify rollup AVG is 14/3 and distinguish a missing outer-join state from COUNT's empty state.
    • No need to test or manual test. Explain why:
  • Behavior changed:
    • No.
    • Yes. Add a family of scalar finalization functions; MV rollup matching now preserves complete value expressions; existing aggregate/state evaluation semantics are unchanged.
  • Does this need documentation?
    • No.
    • Yes. Usage and semantics are included in the function-combinator README.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

Additional validation

  • Standard ASAN BE + FE build, FE Checkstyle, clang-format 16, build hygiene and source whitespace checks passed.
  • clang-tidy was attempted with the repository script and the production/test translation units. New-code style and added cognitive-complexity warnings were fixed. A fully clean run remains blocked by existing header diagnostics and analyzer issues (including a test-helper array-bound path that does not connect its asserted unary arity to the input array); the local tool's crashing modernize-use-scoped-lock check was disabled only for the supplemental analysis. No repository analysis configuration was changed.

Batch state merge validation

  • ./build.sh --be -j 48 completed successfully under ASAN.
  • 19 BE unit tests passed across AggregateStateUnionTest, AggregateStateCombineTest and FunctionAggStateFinalizeTest. Six new tests cover repeated groups, nonzero offsets, empty/all-unselected batches, repeated batches, plain/Nullable/all-NULL SUM, Decimal SUM, string ARRAY_AGG and nullable string MIN. The Nullable selected-row test fails before its reader fix and passes afterward.
  • test_agg_state_union_batch and test_agg_state_finalize both passed in normal regression comparison mode. New expected output was generated with the standard runner and independently checked against ordinary aggregates, arithmetic sums, Decimal sums and lexicographic MIN. The new suite also verifies that plans retain sum_merge/sum_union, and compares ARRAY_AGG results after multiple input batches.
  • clang-format 16.0.6, header hygiene and source whitespace checks passed. clang-tidy was run via the repository script; a fully clean run remains blocked by existing diagnostics in types.h (unmatched NOLINTEND) and DataTypeAggState's constructor, plus existing signed/unsigned comparisons in the Nullable wrapper. No unrelated analysis configuration/source changes were made.
  • Q67 A/B performance has not been measured.

### What problem does this PR solve?

Problem Summary:

A query that has already computed one aggregate state per key still needs an aggregate operator to obtain its final values through `<agg>_merge`. This repeats aggregation for the finest grouping when states are also reused for coarser rollups.

Add the scalar `<agg>_finalize(state)` combinator. It returns one result for each input state, using the existing aggregate implementation and serialized-state representation. For example:

```sql
SELECT k, avg_finalize(s)
FROM (SELECT k, avg_combine(v) AS s FROM t GROUP BY k) partial;
```

The same implementation supports aggregates such as `count`, `sum`, `min`, `max`, and `array_agg`. FE validates that the state's canonical aggregate name matches the finalizer, derives its result type, and treats the function as scalar. BE handles the underlying serialized column type, skips outer NULL payloads, and releases temporary state after each row. Constant inputs use the ordinary scalar constant path. Existing empty-state semantics and serialized formats are unchanged.

This adds the scalar building block only; it does not change optimizer rollup rewrites.

### Release note

Add `<aggregate>_finalize(AGG_STATE)` scalar functions to retrieve each aggregate state's result without merging rows.

### Check List (For Author)

- Test
    - [x] Regression test: test_agg_state_finalize on a fresh local ASAN BE + FE cluster; output generated by the standard runner, checked against direct original aggregates, and verified by a normal comparison run.
    - [x] Unit Test: 33 FE tests passed across FinalizeCombinatorTest, StateCombinatorTest, CombineCombinatorTest, and FunctionRegistryTest. All 7 FunctionAggStateFinalizeTest cases passed under ASAN.
    - [x] Manual test: compare grouped and empty AVG/COUNT/SUM/MIN/MAX, decimal AVG and ARRAY_AGG with the original aggregates; verify rollup AVG is 14/3 and distinguish a missing outer-join state from COUNT's empty state.
    - [ ] No need to test or manual test. Explain why:
- Behavior changed:
    - [ ] No.
    - [x] Yes. Add a family of scalar finalization functions; existing aggregate/state semantics are unchanged.
- Does this need documentation?
    - [ ] No.
    - [x] Yes. Usage and semantics are included in the function-combinator README.

### Check List (For Reviewer who merge this PR)

- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label

### Additional validation

- Standard ASAN BE + FE build, FE Checkstyle, clang-format 16, build hygiene and source whitespace checks passed.
- clang-tidy was attempted with the repository script and the production/test translation units. New-code style and added cognitive-complexity warnings were fixed. A fully clean run remains blocked by existing header diagnostics and analyzer issues (including a test-helper array-bound path that does not connect its asserted unary arity to the input array); the local tool's crashing `modernize-use-scoped-lock` check was disabled only for the supplemental analysis. No repository analysis configuration was changed.
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@HappenLee

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Request changes: two correctness issues block the new scalar finalizer.

  1. Valid stored AGG_STATE schemas that use registered aggregate aliases outside the hand-maintained FE alias map cannot be finalized.
  2. Marking the scalar finalizer as the generic Combinator can corrupt MV aggregate-rollup matching and select an incompatible state.

Checkpoint conclusions: the implementation is focused and the ordinary FE/BE path is coherent, but the stored-schema and optimizer boundaries above are not safe. Null propagation, per-row state lifecycle, exception cleanup, result ownership, concurrent execution, and bounded arena reuse were traced without another defect. The change adds no configuration, persistence, data-write, serialized-format, or observability surface; an old BE fails visibly on the new suffix rather than silently misexecuting it. Test coverage is broad for direct/canonical states, nullable/constant inputs, complex results, and regression outputs, but it misses an omitted stored alias and nested-finalizer MV matching. No additional user focus was supplied.

This was a static review only, as required by the review bundle; I did not run builds or tests.


/** Finalize each serialized aggregate state without aggregating rows. */
public class FinalizeCombinator extends ScalarFunction
implements UnaryExpression, ExplicitlyCastableSignature, ComputeNullable, Combinator {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Combinator is also the marker used by MV aggregate rollup, whose extractLastExpression(..., Combinator.class) returns the deepest matching node. With this scalar marker, a query sum_combine(avg_finalize(s)) and a view max_combine(avg_finalize(s)) both select the inner avg_finalize, so BothCombinatorRollupHandler compares identical nested Avg functions and accepts the incompatible view while ignoring the outer sum versus max. It then builds sum_union over the MV's max state; FE accepts that shape, but BE rejects the state/function mismatch during prepare, so choosing the MV makes an otherwise valid query fail. Please keep scalar finalizers out of the aggregate-rollup marker path or make rollup extraction select the intended aggregate combinator, and add a nested-finalizer MV test.

// AggStateType canonicalizes aliases; acceptsType alone accepts unrelated aggregate states.
AggStateType expected = new AggStateType(nested.getName(), inputType.getSubTypes(),
inputType.getSubTypeNullables(), nested.nullable());
if (!inputType.getFunctionName().equals(expected.getFunctionName())) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A stored state can legally be declared as AGG_STATE<std(DOUBLE)>: the type parser accepts every registered aggregate alias, including std. But AggStateType.aliasToName does not map std, while resolving either std_finalize or stddev_finalize builds a Stddev whose canonical name is stddev, so this comparison rejects the matching stored state before execution. The current alias tests only use directly produced states (already canonical) or aliases present in the hand-maintained map; they miss this stored-schema boundary. The BE prepare check also assumes the raw names match. Please use one complete/authoritative aggregate alias canonicalization path across stored types and FE/BE validation, and add a stored-state case for an omitted alias such as std.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out the stored-alias case. We'll keep the current strict aggregate-name validation in this PR and defer support for aliases missing from the canonicalization map, such as std.

Added a TODO next to the FE check in c620bed661c5 to record the limitation and the follow-up to unify aggregate alias canonicalization across FE and BE. The validation behavior remains unchanged.

### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#68312

Problem Summary: The finalizer's name-check comment implied that all aggregate
aliases were canonicalized. Keep strict state/function name validation and add
a TODO documenting that stored aliases missing from AggStateType's mapping,
such as std, remain unsupported until FE and BE canonicalization is unified.

### Release note

None

### Check List (For Author)

- Test: Maven Checkstyle and git diff --check
    - No need to run runtime tests: comments only, with no behavior change
- Behavior changed: No
- Does this need documentation: No
…llups

### What problem does this PR solve?

Related PR: apache#68312

Problem Summary: MV rollup matching searched the entire argument tree for the
last combinator. A scalar finalizer such as avg_finalize(avg_state(v)) inside
SUM and MAX could make their outer aggregate states appear equivalent, and
scalar wrappers such as ABS could be ignored. Matching plain aggregates with
COMBINE states could also fail for otherwise identical value expressions.

Follow only direct MERGE/UNION state chains for the same aggregate, stopping
at STATE/COMBINE so their complete value expressions remain part of equality.
Use the shared helper in both rollup handlers. Add FE tests and a regression
suite covering invalid candidates, successful rollups, negative values and
NULLs. Six of the nine new FE tests fail before the fix and all pass after it.

### Release note

Fix materialized view rollup matching for aggregate states with nested scalar
finalizers, preserving the outer aggregate and complete value expressions.

### Check List (For Author)

- Test: 51 targeted FE unit tests passed; agg_state_finalize_roll_up and
  test_agg_state_finalize regression suites passed on local ASAN BE + rebuilt
  FE. Regression output generated with the standard runner and independently
  checked. Standard FE build, Checkstyle and source whitespace checks passed.
- Behavior changed: Yes. Reject incompatible state rollups and retain valid
  rollups over identical finalized value expressions.
- Does this need documentation: No. Correctness fix for existing MV rewrites.
@HappenLee

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Request changes: one correctness and compatibility blocker remains on the current head.

A legal stored state such as AGG_STATE<std(DOUBLE)> still cannot be finalized because the hand-maintained FE alias map omits std, while the rebuilt aggregate is named stddev; FE or BE strict name validation rejects the matching state. The author explicitly deferred this behavior rather than fixing it in this PR. This is already covered by the existing inline thread, so I am not posting a duplicate inline comment.

The current head does address the other prior blocker: the MV rollup extractor now preserves value-consuming STATE/COMBINE and scalar FINALIZE boundaries, rejects mismatched outer aggregates, and retains valid same-function MERGE/UNION chains. Two convergence rounds found no additional actionable issue.

Checkpoint conclusions:

  • Goal and implementation: the scalar one-state-per-row finalizer is focused, and direct/canonical state discovery, FE translation, BE reconstruction, exact return typing, and MV rollup behavior are otherwise coherent.
  • Correctness and lifecycle: outer NULL/constant handling, empty states, per-row create/deserialize/finalize/destroy ordering, exception cleanup, variable-length result ownership, and bounded Arena reuse were traced without another defect. There is no new shared mutable state, threading, locking, persistence, transaction, or data-write path.
  • Compatibility and interfaces: existing AggState thrift/storage metadata and execution-version binding are reused; no serialized-format or configuration field changes. The unresolved stored-alias boundary is the remaining compatibility failure. Parallel STATE/COMBINE/MERGE/UNION paths use the same nested aggregate contract.
  • Errors, performance, and observability: name/type mismatches fail explicitly; per-row aggregate work is inherent to the feature, and no separate hot-path or observability issue was substantiated.
  • Tests: coverage is broad for direct, combined, stored canonical, nullable, constant, empty, decimal, complex-result, parameterized, union, invalid, and MV cases, but it still lacks and does not support a stored omitted alias such as std.

No additional user focus was supplied. This was a static review only as required by the review bundle; I did not run builds or tests.

@HappenLee

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 27617 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 6e8121543b0548c42887f8bddf23b64ed00a4274, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17599	3784	3757	3757
q2	2665	374	308	308
q3	10323	1371	795	795
q4	4689	471	348	348
q5	7668	802	544	544
q6	179	175	133	133
q7	723	771	599	599
q8	9422	1494	1547	1494
q9	6225	4131	4153	4131
q10	7537	1313	1009	1009
q11	479	267	232	232
q12	634	406	287	287
q13	18117	2577	2007	2007
q14	253	254	230	230
q15	q16	732	708	652	652
q17	1706	1047	1017	1017
q18	6461	5584	5555	5555
q19	1179	1216	1104	1104
q20	480	393	256	256
q21	5745	3107	2847	2847
q22	442	356	312	312
Total cold run time: 103258 ms
Total hot run time: 27617 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4564	4401	4374	4374
q2	712	558	541	541
q3	4728	5293	4573	4573
q4	2201	2338	1456	1456
q5	4534	4461	4514	4461
q6	224	169	135	135
q7	1808	1669	1506	1506
q8	2306	2030	2042	2030
q9	7361	7008	6869	6869
q10	3620	3540	3067	3067
q11	522	369	336	336
q12	703	709	514	514
q13	2280	2595	1999	1999
q14	265	269	248	248
q15	q16	663	681	596	596
q17	7231	6643	6573	6573
q18	11918	11102	11705	11102
q19	1100	997	1005	997
q20	2196	2203	1897	1897
q21	4933	4040	4233	4040
q22	506	465	395	395
Total cold run time: 64375 ms
Total hot run time: 57709 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152073 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 6e8121543b0548c42887f8bddf23b64ed00a4274, data reload: false

query5	4336	600	455	455
query6	438	213	187	187
query7	4977	559	292	292
query8	332	178	175	175
query9	8791	3993	4019	3993
query10	453	304	258	258
query11	5800	3542	3239	3239
query12	143	89	82	82
query13	1237	580	417	417
query14	6529	4516	4229	4229
query14_1	3929	3963	3902	3902
query15	204	196	174	174
query16	982	437	420	420
query17	867	655	547	547
query18	2433	451	322	322
query19	195	196	134	134
query20	82	79	80	79
query21	221	133	117	117
query22	13067	13010	12798	12798
query23	13875	12907	12301	12301
query23_1	12405	12511	12412	12412
query24	7362	1157	646	646
query24_1	682	772	684	684
query25	546	396	348	348
query26	1139	318	161	161
query27	2674	564	328	328
query28	4513	1950	1939	1939
query29	1534	719	507	507
query30	309	218	178	178
query31	878	758	624	624
query32	138	94	94	94
query33	521	289	238	238
query34	1188	1104	638	638
query35	715	740	639	639
query36	790	814	683	683
query37	141	102	95	95
query38	1826	1762	1685	1685
query39	683	673	656	656
query39_1	654	648	664	648
query40	226	120	114	114
query41	65	65	61	61
query42	96	95	93	93
query43	330	346	294	294
query44	1353	694	720	694
query45	180	174	161	161
query46	1067	1188	714	714
query47	1469	1514	1383	1383
query48	408	408	277	277
query49	588	428	303	303
query50	1028	340	258	258
query51	10759	10391	10340	10340
query52	95	94	78	78
query53	238	254	182	182
query54	269	226	195	195
query55	78	74	72	72
query56	244	237	228	228
query57	1457	1452	1313	1313
query58	285	265	258	258
query59	1992	2060	1876	1876
query60	288	247	239	239
query61	170	171	170	170
query62	403	324	272	272
query63	218	174	182	174
query64	2786	1107	935	935
query65	3448	3379	3404	3379
query66	1764	431	320	320
query67	20101	19910	20173	19910
query68	3338	1586	1001	1001
query69	422	316	266	266
query70	944	852	870	852
query71	299	250	213	213
query72	2815	2637	2228	2228
query73	879	828	437	437
query74	4774	4596	4336	4336
query75	2341	2293	1969	1969
query76	2319	1139	759	759
query77	358	387	291	291
query78	9333	9076	8498	8498
query79	1328	1244	720	720
query80	564	458	351	351
query81	548	315	279	279
query82	620	157	124	124
query83	308	214	192	192
query84	314	144	119	119
query85	827	477	392	392
query86	327	232	218	218
query87	2010	1999	1850	1850
query88	3585	2688	2633	2633
query89	357	287	237	237
query90	1920	185	178	178
query91	169	156	127	127
query92	104	82	83	82
query93	1454	1417	818	818
query94	556	342	284	284
query95	665	447	345	345
query96	1075	761	335	335
query97	2446	2428	2301	2301
query98	161	150	140	140
query99	727	723	614	614
Total cold run time: 236831 ms
Total hot run time: 152073 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 6e8121543b0548c42887f8bddf23b64ed00a4274, data reload: false

query1	0.00	0.00	0.00
query2	0.09	0.04	0.04
query3	0.26	0.14	0.14
query4	1.60	0.14	0.14
query5	0.24	0.21	0.21
query6	1.15	0.91	0.92
query7	0.04	0.01	0.01
query8	0.06	0.04	0.04
query9	0.40	0.32	0.33
query10	0.53	0.55	0.53
query11	0.20	0.14	0.15
query12	0.19	0.15	0.15
query13	0.46	0.47	0.47
query14	0.96	0.95	0.93
query15	0.62	0.59	0.59
query16	0.31	0.33	0.33
query17	1.10	1.12	1.12
query18	0.21	0.20	0.20
query19	2.04	1.96	1.91
query20	0.02	0.02	0.01
query21	15.48	0.23	0.14
query22	4.79	0.05	0.05
query23	16.15	0.30	0.11
query24	3.02	0.43	0.31
query25	0.11	0.06	0.03
query26	0.75	0.22	0.15
query27	0.03	0.04	0.03
query28	3.47	0.75	0.35
query29	12.52	4.10	3.26
query30	0.28	0.15	0.15
query31	2.78	0.58	0.32
query32	3.23	0.60	0.50
query33	3.17	3.17	3.17
query34	15.70	3.93	3.24
query35	3.22	3.22	3.27
query36	0.56	0.42	0.43
query37	0.10	0.07	0.06
query38	0.06	0.03	0.03
query39	0.04	0.03	0.03
query40	0.18	0.15	0.15
query41	0.09	0.03	0.03
query42	0.04	0.03	0.03
query43	0.05	0.04	0.03
Total cold run time: 96.3 s
Total hot run time: 24 s

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 88.64% (39/44) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 63.78% (29675/46525)
Line Coverage 48.48% (309185/637814)
Region Coverage 43.98% (249142/566458)
Branch Coverage 45.57% (115863/254225)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 91.67% (44/48) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 88.64% (39/44) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 76.22% (34339/45051)
Line Coverage 61.12% (385409/630538)
Region Coverage 57.55% (324656/564174)
Branch Coverage 58.35% (147880/253438)

### What problem does this PR solve?

Problem Summary: Scalar aggregate-state finalization allocates aligned state
storage from the per-row arena for every non-NULL input. Allocate this fixed
storage once in a separate arena and reserve the output column capacity. Keep
per-row construction, destruction, and temporary arena reclamation unchanged
so variable-length state data cannot accumulate across rows or invalidate the
reused storage. Add coverage with outer NULLs and arena-growing string states.
The generic deserialization scratch allocation and merge remain unchanged;
no quantitative performance improvement has been measured.

### Release note

None

### Check List (For Author)

- Test: Unit Test - FunctionAggStateFinalizeTest, 8 ASAN cases passed;
  clang-format 16 and build hygiene passed. Targeted clang-tidy was attempted
  but reports existing diagnostics in unchanged core/types.h and
  data_type_agg_state.h; the crashing modernize-use-scoped-lock check was
  disabled locally. No performance benchmark was run.
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?

Related PR: apache#68312

Problem Summary: Scalar aggregate-state finalization constructs and destroys
state for every non-NULL row even when is_trivial() guarantees that zero-init
is sufficient and destruction can be skipped. Dispatch once outside the row
loop and use a compile-time lifecycle branch: memset trivial states for each
row, and retain scoped create/destroy for non-trivial states. Reuse the same
row deserialization and result insertion logic in both paths. Keep fixed
state storage separate from the per-row arena and preserve outer NULL
handling. Add coverage for valid, NULL, empty and negative SUM states and
repeated execution. This removes two lifecycle virtual calls per non-NULL
trivial row; no quantitative performance improvement has been measured.

### Release note

None

### Check List (For Author)

- Test: Unit Test - all 9 FunctionAggStateFinalizeTest cases passed under
  ASAN via run-be-ut.sh -j 48 --run --filter=FunctionAggStateFinalizeTest.*.
  build.sh --be -j 48, clang-format 16, build hygiene and whitespace checks
  passed. Targeted clang-tidy was attempted but is blocked by pre-existing
  diagnostics in core/types.h and data_type_agg_state.h. No performance
  benchmark or SQL regression suite was run for this follow-up.
- Behavior changed: No
- Does this need documentation: No
@feiniaofeiafei

Copy link
Copy Markdown
Collaborator

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 27771 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 6fd8235dfbf2375429a22c1d3681a8c26a9c9344, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17762	3885	3912	3885
q2	2099	359	328	328
q3	10147	1440	782	782
q4	4685	512	352	352
q5	7524	830	566	566
q6	178	167	134	134
q7	744	781	586	586
q8	9344	1447	1496	1447
q9	5440	4186	4140	4140
q10	6824	1337	1012	1012
q11	441	280	244	244
q12	641	408	296	296
q13	18046	2628	1994	1994
q14	264	270	239	239
q15	q16	736	721	673	673
q17	1885	1174	986	986
q18	6592	5628	5555	5555
q19	1314	1225	1012	1012
q20	495	398	269	269
q21	5867	3413	2955	2955
q22	455	368	316	316
Total cold run time: 101483 ms
Total hot run time: 27771 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4686	4698	4494	4494
q2	737	578	537	537
q3	4744	5115	4570	4570
q4	2225	2313	1457	1457
q5	4660	4384	4555	4384
q6	231	172	120	120
q7	1797	1706	1500	1500
q8	2368	2089	2046	2046
q9	7303	7371	7146	7146
q10	3705	3612	3137	3137
q11	513	377	336	336
q12	705	706	497	497
q13	2296	2616	2002	2002
q14	280	272	242	242
q15	q16	666	680	599	599
q17	7285	6701	6650	6650
q18	11928	11086	11768	11086
q19	1086	981	988	981
q20	2211	2176	1911	1911
q21	4996	4115	4291	4115
q22	503	483	396	396
Total cold run time: 64925 ms
Total hot run time: 58206 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 152358 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 6fd8235dfbf2375429a22c1d3681a8c26a9c9344, data reload: false

query5	4367	608	479	479
query6	453	221	203	203
query7	4816	584	292	292
query8	325	188	165	165
query9	8837	3932	3893	3893
query10	429	326	250	250
query11	5865	3547	3200	3200
query12	142	87	86	86
query13	1253	611	413	413
query14	6514	4532	4162	4162
query14_1	3954	3925	3914	3914
query15	205	194	192	192
query16	981	455	410	410
query17	874	674	505	505
query18	2447	461	328	328
query19	190	174	158	158
query20	79	81	77	77
query21	217	130	114	114
query22	13046	13091	12758	12758
query23	14015	12953	12506	12506
query23_1	12644	12599	12426	12426
query24	7282	1130	640	640
query24_1	695	683	717	683
query25	541	409	355	355
query26	1240	293	160	160
query27	2716	589	340	340
query28	4583	1966	1984	1966
query29	1584	714	529	529
query30	299	220	184	184
query31	921	767	641	641
query32	148	109	100	100
query33	520	310	245	245
query34	1187	1098	637	637
query35	732	755	647	647
query36	805	808	711	711
query37	145	108	94	94
query38	1835	1758	1691	1691
query39	668	683	666	666
query39_1	660	656	628	628
query40	229	127	108	108
query41	72	75	69	69
query42	97	100	94	94
query43	337	352	299	299
query44	1348	725	726	725
query45	191	185	165	165
query46	1091	1185	702	702
query47	1512	1507	1409	1409
query48	395	396	264	264
query49	591	392	283	283
query50	979	356	253	253
query51	10503	10577	10270	10270
query52	88	88	77	77
query53	237	248	180	180
query54	239	203	186	186
query55	78	72	71	71
query56	221	213	206	206
query57	1473	1432	1362	1362
query58	279	258	248	248
query59	1973	2068	1867	1867
query60	278	240	226	226
query61	149	148	148	148
query62	403	323	275	275
query63	216	179	176	176
query64	2809	1028	818	818
query65	3490	3399	3436	3399
query66	1818	425	302	302
query67	19872	20064	19976	19976
query68	3283	1513	960	960
query69	395	289	249	249
query70	892	811	810	810
query71	290	229	212	212
query72	2603	2562	2218	2218
query73	843	758	417	417
query74	4637	4480	4318	4318
query75	2297	2291	1952	1952
query76	2373	1117	765	765
query77	366	408	320	320
query78	8947	8960	8474	8474
query79	1242	1164	739	739
query80	528	513	397	397
query81	533	339	286	286
query82	276	172	132	132
query83	232	230	205	205
query84	298	147	122	122
query85	867	531	453	453
query86	277	238	228	228
query87	1996	1965	1820	1820
query88	3599	2763	2732	2732
query89	329	287	252	252
query90	2152	188	184	184
query91	186	172	145	145
query92	106	94	97	94
query93	1388	1416	860	860
query94	558	343	301	301
query95	693	395	354	354
query96	1105	740	357	357
query97	2427	2425	2318	2318
query98	169	193	145	145
query99	720	725	625	625
Total cold run time: 235287 ms
Total hot run time: 152358 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 24.03 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 6fd8235dfbf2375429a22c1d3681a8c26a9c9344, data reload: false

query1	0.01	0.01	0.00
query2	0.10	0.05	0.05
query3	0.27	0.13	0.13
query4	1.61	0.15	0.14
query5	0.25	0.22	0.22
query6	1.16	0.92	0.95
query7	0.04	0.00	0.00
query8	0.06	0.04	0.04
query9	0.38	0.33	0.34
query10	0.55	0.55	0.57
query11	0.20	0.14	0.14
query12	0.18	0.14	0.14
query13	0.46	0.46	0.47
query14	0.93	0.94	0.93
query15	0.62	0.58	0.58
query16	0.34	0.33	0.32
query17	1.12	1.11	1.11
query18	0.21	0.20	0.20
query19	2.05	1.89	1.93
query20	0.02	0.01	0.02
query21	15.43	0.21	0.15
query22	4.84	0.06	0.05
query23	16.12	0.31	0.12
query24	2.98	0.44	0.33
query25	0.11	0.05	0.04
query26	0.73	0.22	0.14
query27	0.04	0.03	0.03
query28	3.49	0.78	0.35
query29	12.49	4.12	3.19
query30	0.27	0.16	0.15
query31	2.77	0.55	0.32
query32	3.25	0.59	0.49
query33	3.25	3.19	3.26
query34	15.54	3.97	3.29
query35	3.24	3.23	3.25
query36	0.57	0.44	0.44
query37	0.09	0.07	0.06
query38	0.05	0.05	0.03
query39	0.04	0.03	0.03
query40	0.18	0.15	0.15
query41	0.09	0.03	0.03
query42	0.04	0.03	0.03
query43	0.04	0.04	0.03
Total cold run time: 96.21 s
Total hot run time: 24.03 s

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 91.23% (52/57) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 63.95% (29771/46550)
Line Coverage 48.59% (310067/638129)
Region Coverage 44.10% (249933/566712)
Branch Coverage 45.68% (116199/254401)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 91.23% (52/57) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 76.15% (34326/45076)
Line Coverage 61.06% (385187/630853)
Region Coverage 57.40% (323953/564427)
Branch Coverage 58.26% (147755/253614)

@feiniaofeiafei

Copy link
Copy Markdown
Collaborator

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Request changes: a distinct FE/BE return-type contract failure remains on exact head 6fd8235dfbf2375429a22c1d3681a8c26a9c9344.

The inline P1 covers decimal state finalization: AggState metadata retains argument subtypes but not the effective result type, so BE can reconstruct a different SUM/AVG return type from FE. Valid scalar states in Decimal256 mode and decimal SUM_MAP/AVG_MAP states in default mode then fail the new exact prepare check. The stored std alias limitation remains covered by the existing thread and is not duplicated. The prior MV deepest-combinator issue is addressed on this head.

Critical checkpoint conclusions:

  • Goal and proof: the PR adds per-row scalar finalization of one serialized aggregate state and updates MV combinator matching. Direct, stored canonical, nullable, constant, empty, complex-result, parameterized, invalid, and MV paths have focused FE/BE/unit/regression coverage, but the goal is incomplete for the decimal cases in the inline finding.
  • Scope: the implementation is otherwise focused and reuses the existing aggregate/state factories and expression translation paths.
  • Concurrency: no new threads, shared mutable state, locks, atomics, or lock ordering are introduced; function execution uses call-local state and arenas.
  • Lifecycle/static initialization: per-row create/deserialize/finalize/destroy ordering, trivial zero-init, exception unwinding, arena-clear ordering, zero-row behavior, and variable-length result ownership were traced without another defect. No cross-TU static initialization dependency was added.
  • Configuration: no new configuration item is added. The existing enable_decimal256 flag exposes one manifestation of the accepted FE/BE mismatch.
  • Compatibility: no storage or serialized-state format is changed, and the state execution version is retained. A new FE requires a BE that recognizes the scalar AGG_STATE suffix during rolling deployment.
  • Parallel paths: direct STATE/COMBINE, stored/UNION states, normal scalar translation, BE-backed evaluation, nullable/constant columns, and MV MERGE/UNION handlers were checked; no separate-path defect remains beyond the finding and the fenced alias limitation.
  • Conditional checks: arity, AGG_STATE type, function name, nullability, and exact return-type failures are explicit. The exact return check is reasonable, but its inputs are inconsistent in the reported decimal paths.
  • Test coverage/results: the checked-in expected outputs match the covered queries and repository test conventions. Missing end-to-end narrow Decimal256 SUM/AVG and default-mode decimal SUM_MAP/AVG_MAP finalization would have exposed the blocker.
  • Observability: no new distributed or persistent operation needs metrics; prepare failures carry function and expected/actual types.
  • Persistence, transactions, and data writes: no production metadata persistence, transaction, visibility, or write path is changed.
  • FE/BE data transfer: no new variable is sent, but the effective aggregate result type is not recoverable from the argument-only AggState contract in the reported cases; this is the accepted issue.
  • Performance: the state slot and arenas are reused per block/row, constants are materialized at the existing boundary, and no distinct hot-path regression was substantiated.
  • Other issues: two convergence rounds and the final 18-file sweep found no additional distinct finding.

No additional user focus was supplied. Validation was static-only as required by the review bundle; no builds or tests were run independently. Current CI shows BE UT, FE UT, compile, formatting/checkstyle, P0, and nonconcurrent regression passing, while check_coverage_fe (Coverage) is failing.

if (argument_types[0]->is_nullable()) {
expected_type = make_nullable(expected_type);
}
if (!expected_type->equals(*return_type)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P1] Preserve the effective decimal aggregate result type

This equality check rejects valid states because DataTypeAggState cannot always reconstruct the return type FE planned from argument subtypes alone. For example, with enable_decimal256=true, sum_finalize(sum_state(cast(1 as decimal(12,2)))) and the AVG equivalent have Decimal256 FE return types, but BE rebuilds their narrow state subtype as Decimal128. The problem is not limited to that flag: FE gives decimal sum_map/avg_map a MAP<K,DECIMAL(38,s)> result, while the state stores MAP<K,DECIMAL(12,s)>; since DataTypeAggState only widens a top-level DecimalV3 subtype, BE rebuilds the map's nested aggregate with the narrow decimal family. In both cases nested->get_return_type() differs and prepare returns InternalError. Please preserve the effective nested result type in the state/reconstruction contract (or otherwise make FE and BE derive it identically), with end-to-end scalar and MAP decimal finalizer coverage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

enable_decimal256 is disabled by default. Given that default configuration, I suggest treating the Decimal256-specific case as non-blocking for this PR and addressing Decimal256 AggState support separately.

The runtime type-consistency check should remain in place. The decimal MAP case mentioned above is independent of this flag, so the default being disabled does not address that separate case.

### What problem does this PR solve?

Issue Number: N/A

Related PR: apache#68312

Problem Summary: Grouped AggregateStateUnion invokes a range merge for each
input row even when the nested aggregate already supports vectorized merging.
Delegate regular and selected batches to the existing interfaces, also
benefiting AggregateStateMerge. Allocate aligned temporary state storage in a
batch-local Arena while preserving the caller Arena for retained allocations.
Document ownership and keep single-place merging unchanged.

The reused Nullable selected fast path previously skipped destinations without
advancing its shared serialized-input reader. Read selected rows independently
to preserve row-to-group mapping. Tests compare the batch paths against the
prior per-row path and ordinary SQL aggregates. Q67 performance is unmeasured.

### Release note

Use batch merge interfaces for grouped aggregate-state unions and merges, and
preserve selected-row alignment when merging Nullable aggregate states.

### Check List (For Author)

- Test: ASAN BE build; 19 BE unit tests; 2 SQL regression suites; independent
  expected-result checks; clang-format 16.0.6; header hygiene. clang-tidy attempted,
  with existing header diagnostics preventing a fully clean run.
- Behavior changed: Yes. Use batch merging and correct Nullable selected-row
  decoding; aggregate results and state serialization remain compatible.
- Does this need documentation: No. Internal implementation comments included.
@HappenLee HappenLee changed the title [feature](be) Add scalar aggregate state finalize functions [feature](be) Add aggregate state finalizers and batch merging Sep 23, 2026
@HappenLee

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 27985 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit b9a7047e0766a2240b7944c2aa8b594942f93f9f, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17799	3965	3931	3931
q2	2277	368	306	306
q3	10031	1369	807	807
q4	4695	472	353	353
q5	7474	832	548	548
q6	178	177	134	134
q7	718	781	602	602
q8	9364	1613	1500	1500
q9	5429	4174	4128	4128
q10	6826	1363	1028	1028
q11	445	265	238	238
q12	633	412	291	291
q13	18100	2650	2011	2011
q14	262	256	239	239
q15	q16	728	718	655	655
q17	1700	1121	1002	1002
q18	6495	5614	5600	5600
q19	1182	1163	1016	1016
q20	462	385	265	265
q21	5090	3192	3011	3011
q22	446	377	320	320
Total cold run time: 100334 ms
Total hot run time: 27985 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	4691	4678	4480	4480
q2	719	566	536	536
q3	5007	5091	4629	4629
q4	2184	2342	1453	1453
q5	4561	4366	4574	4366
q6	220	211	124	124
q7	1789	1696	1455	1455
q8	2317	2024	1991	1991
q9	7209	7181	7133	7133
q10	3627	3544	3072	3072
q11	512	372	346	346
q12	705	698	503	503
q13	2260	2599	1994	1994
q14	263	275	257	257
q15	q16	665	677	594	594
q17	7274	6676	6635	6635
q18	12076	11087	11733	11087
q19	1105	990	1031	990
q20	2203	2178	1898	1898
q21	4978	4093	4280	4093
q22	513	448	413	413
Total cold run time: 64878 ms
Total hot run time: 58049 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 151774 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit b9a7047e0766a2240b7944c2aa8b594942f93f9f, data reload: false

query5	4338	590	432	432
query6	425	207	190	190
query7	4823	568	273	273
query8	326	179	161	161
query9	8813	3976	3925	3925
query10	446	300	259	259
query11	5847	3495	3232	3232
query12	152	89	85	85
query13	1240	560	410	410
query14	6531	4455	4184	4184
query14_1	3972	3918	3906	3906
query15	205	195	188	188
query16	986	448	410	410
query17	923	664	547	547
query18	2433	467	344	344
query19	213	178	148	148
query20	83	82	81	81
query21	229	134	118	118
query22	13029	13055	12732	12732
query23	13856	13028	12532	12532
query23_1	12573	12469	12561	12469
query24	7253	1087	669	669
query24_1	661	723	700	700
query25	559	434	371	371
query26	1255	318	170	170
query27	2795	586	331	331
query28	4563	1949	1945	1945
query29	1609	749	505	505
query30	294	217	181	181
query31	885	748	640	640
query32	146	87	89	87
query33	525	297	223	223
query34	1177	1116	627	627
query35	702	733	620	620
query36	823	802	728	728
query37	148	111	98	98
query38	1837	1744	1690	1690
query39	689	703	635	635
query39_1	661	639	649	639
query40	217	119	99	99
query41	64	65	63	63
query42	92	89	90	89
query43	332	347	292	292
query44	1368	702	715	702
query45	184	173	162	162
query46	1071	1151	715	715
query47	1472	1486	1394	1394
query48	397	409	286	286
query49	571	411	285	285
query50	924	336	249	249
query51	10311	10693	10117	10117
query52	86	88	76	76
query53	245	252	178	178
query54	263	208	184	184
query55	77	72	72	72
query56	226	202	202	202
query57	1429	1446	1396	1396
query58	297	261	257	257
query59	1951	2026	1850	1850
query60	285	237	221	221
query61	141	150	145	145
query62	392	313	265	265
query63	217	181	186	181
query64	2796	1001	845	845
query65	3486	3389	3382	3382
query66	1799	430	310	310
query67	20144	20148	20038	20038
query68	3349	1441	923	923
query69	408	305	257	257
query70	870	816	831	816
query71	288	223	209	209
query72	2719	2570	2229	2229
query73	805	740	429	429
query74	4698	4484	4306	4306
query75	2297	2289	1925	1925
query76	2367	1076	728	728
query77	352	402	300	300
query78	9004	9020	8375	8375
query79	1164	1135	746	746
query80	505	485	370	370
query81	519	320	284	284
query82	251	158	123	123
query83	209	223	194	194
query84	289	142	116	116
query85	801	472	398	398
query86	301	250	227	227
query87	1957	1945	1804	1804
query88	3568	2711	2691	2691
query89	320	297	247	247
query90	2149	176	172	172
query91	169	159	129	129
query92	103	90	87	87
query93	1346	1435	830	830
query94	518	339	298	298
query95	640	458	332	332
query96	1014	768	336	336
query97	2440	2436	2336	2336
query98	176	148	147	147
query99	709	729	619	619
Total cold run time: 234816 ms
Total hot run time: 151774 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 23.97 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit b9a7047e0766a2240b7944c2aa8b594942f93f9f, data reload: false

query1	0.00	0.00	0.00
query2	0.09	0.05	0.05
query3	0.26	0.13	0.14
query4	1.61	0.14	0.14
query5	0.25	0.22	0.22
query6	1.16	0.96	0.94
query7	0.04	0.00	0.00
query8	0.05	0.03	0.04
query9	0.40	0.34	0.33
query10	0.56	0.57	0.55
query11	0.21	0.14	0.15
query12	0.19	0.15	0.15
query13	0.47	0.46	0.47
query14	0.95	0.96	0.97
query15	0.61	0.60	0.58
query16	0.30	0.31	0.31
query17	1.11	1.04	1.05
query18	0.24	0.20	0.20
query19	2.03	1.98	1.90
query20	0.02	0.01	0.01
query21	15.55	0.20	0.14
query22	4.84	0.05	0.06
query23	16.14	0.31	0.13
query24	2.96	0.40	0.32
query25	0.12	0.05	0.04
query26	0.74	0.20	0.15
query27	0.05	0.04	0.04
query28	3.47	0.82	0.37
query29	12.50	4.12	3.21
query30	0.28	0.15	0.16
query31	2.76	0.54	0.31
query32	3.22	0.58	0.48
query33	3.20	3.17	3.19
query34	15.58	3.90	3.29
query35	3.24	3.21	3.24
query36	0.56	0.42	0.42
query37	0.09	0.06	0.06
query38	0.05	0.03	0.04
query39	0.05	0.03	0.03
query40	0.18	0.15	0.15
query41	0.09	0.03	0.03
query42	0.04	0.02	0.04
query43	0.04	0.03	0.04
Total cold run time: 96.3 s
Total hot run time: 23.97 s

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 92.31% (72/78) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 65.02% (30761/47312)
Line Coverage 49.82% (323084/648531)
Region Coverage 45.31% (261121/576324)
Branch Coverage 46.95% (122422/260774)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 92.31% (72/78) 🎉

Increment coverage report
Complete coverage report

Category Coverage
Function Coverage 76.70% (35121/45789)
Line Coverage 61.92% (397011/641131)
Region Coverage 58.41% (335246/573992)
Branch Coverage 59.25% (154036/259975)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 91.67% (44/48) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 63.08% (41/65) 🎉
Increment coverage report
Complete coverage report

@HappenLee

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Complete static review of PR #68312 at b9a7047. I inspected all 23 changed files and traced FE binding/translation, BE finalization and batch merging, MV rollup, and the related tests. No new inline findings emerged after the normal and focused review passes.

The existing [P1] comment 4079863218 still applies. With default settings, decimal sum_map/avg_map states can retain a narrow nested DECIMAL while FE plans a widened MAP result; create_agg_state_finalize_function reconstructs the nested result from the state subtype and rejects that planned type at BE prepare. I am carrying that blocker without reposting it. Existing thread 4059519367 still covers the stored std alias limitation; the author deferred it. The earlier nested-finalizer MV concern in 4059519364 is addressed on this head: rollup comparison keeps the outer aggregate and full value expression, with negative MV tests.

Critical checkpoints:

  • Goal, focus, and scope: the scalar *_finalize path and whole-batch *_union merge are wired through FE and BE and are reasonably focused. The known P1 prevents a clean correctness conclusion. There was no additional user-provided focus.
  • Concurrency, lifecycle, memory, and errors: batch scratch is per call; finalizer states are reset or constructed per row and nontrivial states are destroyed before their arena is cleared. Selected rows use independent readers; zero batches return before row-zero access. No new locks, shared mutable state, static initialization dependency, ignored Status, or silent error path was found.
  • Configuration, compatibility, parallel paths, and persistence: no new configuration, FE/BE variable, serialized format, transaction, data-write, or EditLog path was added. The existing enable_decimal256 setting also exposes the already-threaded return-type issue. Direct/stored states, native/string serialization, nullable/constant inputs, and MERGE/UNION/STATE/COMBINE rollup paths were checked.
  • Performance and observability: native vector merge avoids repeated per-row dispatch; its per-batch scratch allocation and the scalar per-row work revealed no substantiated new regression. Existing prepare errors include function/type details; no new metric or log need was established.
  • Tests and results: added BE/FE unit tests and three regression suites exercise ordinary, negative, nullable, empty, selected, variable-length, and MV cases. The checked expected outputs match those scenarios, but builds and tests were prohibited here and were not run independently. Decimal MAP finalization remains untested and is covered by the existing P1.

Existing P0/P1 findings confirmed for this head: #68312 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants