Conversation
### 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.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Request changes: two correctness issues block the new scalar finalizer.
- Valid stored
AGG_STATEschemas that use registered aggregate aliases outside the hand-maintained FE alias map cannot be finalized. - Marking the scalar finalizer as the generic
Combinatorcan 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 { |
There was a problem hiding this comment.
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())) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
/review |
There was a problem hiding this comment.
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.
|
run buildall |
TPC-H: Total hot run time: 27617 ms |
TPC-DS: Total hot run time: 152073 ms |
ClickBench: Total hot run time: 24 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### 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
|
run buildall |
TPC-H: Total hot run time: 27771 ms |
TPC-DS: Total hot run time: 152358 ms |
ClickBench: Total hot run time: 24.03 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
There was a problem hiding this comment.
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_decimal256flag 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)) { |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
|
run buildall |
TPC-H: Total hot run time: 27985 ms |
TPC-DS: Total hot run time: 151774 ms |
ClickBench: Total hot run time: 23.97 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
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
*_finalizepath and whole-batch*_unionmerge 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_decimal256setting 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)
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:The same implementation supports aggregates such as
count,sum,min,max, andarray_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
*_unionand*_mergenow 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)
Check List (For Reviewer who merge this PR)
Additional validation
modernize-use-scoped-lockcheck was disabled only for the supplemental analysis. No repository analysis configuration was changed.Batch state merge validation
./build.sh --be -j 48completed successfully under ASAN.test_agg_state_union_batchandtest_agg_state_finalizeboth 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.