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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 |
2 participants
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.
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.