[feature](agg_state) Add aggregate state combine combinator - #66942
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
|
run buildall |
TPC-H: Total hot run time: 17395 ms |
TPC-DS: Total hot run time: 83953 ms |
ClickBench: Total hot run time: 14.55 s |
|
/review |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-20T03:35:00Z. Please trigger /review again after that time. |
c9f81a3 to
40b1c15
Compare
|
run buildall |
|
Review summary for rebased head 40b1c15: the combine aggregate-state path is wired through BE and FE, argument/return state compatibility is validated, and FE unit plus regression coverage is included. |
|
/review |
TPC-H: Total hot run time: 17477 ms |
TPC-DS: Total hot run time: 83584 ms |
ClickBench: Total hot run time: 14.81 s |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for seven blocking correctness issues and one P2 error-handling issue; each has a concrete trigger in the inline comments.
Checkpoint conclusions:
- Goal and accomplishment: direct raw aggregation into a reusable
AggStateis a sound optimization, and theavghappy path preserves the existing state layout, but the generic combinator contract is not met for several admitted aggregates and plan shapes. - Scope and focus: all 14 authoritative changed files and the relevant FE builder/translator/planner/MV paths plus BE evaluator/state consumers were reviewed. No additional user focus was supplied.
- Correctness and parallel paths: grouped/no-key, one/two-phase, merge/union, scalar and correlated subqueries, analytic windows, MV creation/roll-up, constructor-expanded overloads, invalid base names, and specialized aggregate policies were traced. The inline findings cover the surviving failures.
- Concurrency: no new locks, threads, or shared mutable ownership were introduced; no concurrency defect was found.
- Lifecycle and resources: create/reset/destroy and serialized-column ownership delegate consistently. The AI failure is initialization, not a leak: required
QueryContextnever reaches the nested function. - Configuration and compatibility: no new config, persisted metadata field, or wire protocol was added, and ordinary state bytes remain compatible with
_state/_union/_merge. Session-selected phase/window behavior and terminal serialization expose correctness gaps called out inline. - Transactions and persistence: no transaction, retry, or durable-write semantics changed. Synchronous MV construction is the relevant storage boundary and currently fails before materialization.
- FE/BE contract: ordinary subtype/arity checks are coherent, but canonical AI children, nullability, phase policy, window attributes, and terminal serialization are not fully propagated.
- Performance: the intended removal of per-row state serialization is preserved on the valid
avgpath; no separate hot-path regression was found. - Tests: the added avg unit/regression coverage checks grouped, nullable, all-null, fixed-length, and compatibility behavior, but misses AI overloads/setup, scalar/correlated subqueries, sliding windows, MV creation, phase-restricted aggregates, non-aggregate suffixes, and terminal two-phase serialization. Per the review prompt, no builds or tests were run locally. Live style, license, secrets, Cloud UT, and performance checks pass; FE UT, BE UT, compile, and code-review remain pending.
- Observability: no new metric/log requirement was identified; several failures occur during analysis/setup or return wrong state values, so tests and explicit validation are the important guardrails.
- Review status: round 3 reached the configured cap and still produced the terminal-serialization finding, so this review is
capped/incomplete, not converged.
| _function->deserialize_and_merge_from_column_range(place, column, begin, end, arena); | ||
| } | ||
|
|
||
| void insert_result_into(ConstAggregateDataPtr __restrict place, IColumn& to) const override { |
There was a problem hiding this comment.
[P1] Preserve merged states with terminal serialization
In the required two-phase path for orthogonal_bitmap_expr_calculate[_count]_combine, local serialization finalizes the raw calculator into a separate result, and global merge accumulates only that field. This finalizer serializes the merged state again; the nested write() then overwrites result from the global state's empty raw calculator, emitting zero or an empty bitmap. The matching _merge therefore returns the wrong value. Please gate _combine on an explicit merge-stable serialization capability or preserve the merged serialized result, and add forced two-phase count and bitmap tests.
There was a problem hiding this comment.
Fixed. The orthogonal bitmap aggregate family now implements NotSupportAggState, so _state, _merge, _union, and _combine are rejected during FE function resolution instead of entering an unsupported terminal-serialization path. The unit test covers all affected orthogonal aggregate classes.
| previous_is_nul, end_is_nul, has_null, use_null_result, could_use_previous_result); | ||
| } | ||
|
|
||
| void set_query_context(QueryContext* context) override { |
There was a problem hiding this comment.
[P1] Propagate QueryContext to the wrapped AI aggregate
FunctionRegistry now accepts ai_agg_combine(...), but AggFnEvaluator::prepare() calls set_query_context() only when the outer name is exactly ai_agg. For ai_agg_combine, this forwarding method is never called; AggregateFunctionAIAgg::create() stores a null context and its first add() dereferences _ctx in prepare(), which can crash the BE. Please key this setup off the nested aggregate/capability (or explicitly handle the combinator) and cover a valid three-argument AI combine.
There was a problem hiding this comment.
Fixed by rejecting ai_agg_state and ai_agg_combine during FE function resolution. AIAgg now implements NotSupportAggStateCreation, which prevents a wrapped AI aggregate without the required query context from reaching BE. Full AI AggState support can be added later after its context and canonical arguments are modeled correctly.
| argument_types[i]->get_name()); | ||
| } | ||
| } | ||
| _function = AggregateStateCombine::create(state_type->get_nested_function(), |
There was a problem hiding this comment.
[P1] Build a window-aware nested state for _combine
This reuses the DataTypeAggState nested function, which is constructed with the default is_window_function=false, even when this evaluator represents an analytic window. The wrapper still forwards incremental support. For nullable Avg, a bounded-frame transition from a non-null row to an all-null frame reaches a window-only DCHECK in debug builds; in release it never tracks null_count and serializes a populated count-zero state, so merging that state produces a non-NULL/NaN result instead of NULL. Please construct a window-aware nested function or disable incremental mode for this case, and test non-null-to-all-null frame transitions.
There was a problem hiding this comment.
Fixed. WindowFunctionChecker now explicitly rejects CombineCombinator, so _combine cannot be planned as a window function until a window-aware nested state is implemented. A unit test verifies the analysis rejection.
| if (!(queryAggregateFunction instanceof Combinator) | ||
| && (viewFunction instanceof UnionCombinator || viewFunction instanceof StateCombinator)) { | ||
| && (viewFunction instanceof UnionCombinator || viewFunction instanceof StateCombinator | ||
| || viewFunction instanceof CombineCombinator)) { |
There was a problem hiding this comment.
[P1] Store combine state directly in synchronous MVs
Allowing CombineCombinator here makes it a valid view-side roll-up expression, but CreateMaterializedViewCommand still treats every generic aggregate as a raw aggregate and wraps it with StateCombinator.create(). For avg_combine(v), that constructs a synthetic avg_combine_state and null-unboxes aggFuncNameNullableMap.get("avg_combine"), so CREATE MATERIALIZED VIEW ... avg_combine(v) fails before rewrite. Please recognize _combine as already state-valued (or reject this MV form deliberately) and cover MV creation plus roll-up.
There was a problem hiding this comment.
Fixed. Synchronous MV analysis now explicitly rejects CombineCombinator instead of storing it through the existing aggregate-state rewrite path. A regression test covers the expected analysis error.
| if (combinatorSuffix.equalsIgnoreCase(COMBINE)) { | ||
| // DataTypeAggState needs at least one subtype, so zero-argument aggregates such as | ||
| // count(*) cannot produce an AggState yet. count_combine(1) remains supported. | ||
| return !arguments.isEmpty() && nestedBuilder.canApply(arguments); |
There was a problem hiding this comment.
[P2] Filter _combine resolution to aggregate builders
This branch only checks the nested builder's signature, while dynamic suffix lookup tries every built-in registered under the base name. Thus abs_combine(1) retains the scalar Abs builder and buildState() later casts the built scalar to AggregateFunction, throwing an internal ClassCastException instead of a normal analysis error. Require the nested builder's class to be an aggregate before advertising this combinator, and add a negative scalar-name test.
There was a problem hiding this comment.
Fixed. AggCombinerFunctionBuilder.canApply first verifies that the nested builder represents an AggregateFunction. Scalar names such as abs_combine now fail normal function resolution instead of reaching the aggregate cast and throwing an internal exception. Covered by unit test.
| if (!arguments.isEmpty() && arguments.get(0) instanceof Boolean && (Boolean) arguments.get(0)) { | ||
| throw new IllegalStateException(name + " doesn't support DISTINCT"); | ||
| } | ||
| return Pair.of(new CombineCombinator((List<Expression>) arguments, nestedFunction), nestedFunction); |
There was a problem hiding this comment.
[P1] Preserve constructor-expanded arguments in the combine expression
AIAgg(text, task) prepends the default resource and canonicalizes itself to three children, but this branch discards nestedFunction.children() and builds CombineCombinator from the original two arguments. The resulting AggStateType and translated TExpr contain only two columns, while the BE AI aggregate unconditionally reads columns[2], causing an out-of-bounds access. Please propagate the nested function's canonical children (while handling the DISTINCT marker) or reject arity-changing builders, and cover the default-resource overload.
There was a problem hiding this comment.
Fixed for this PR by explicitly disabling ai_agg_state and ai_agg_combine at function resolution. This avoids constructing an AggState from the non-canonical two-argument child list while AIAgg expands it to three arguments internally. Canonical AI AggState support is deferred to a follow-up.
| * Aggregate inputs into the nested function's serialized state. | ||
| */ | ||
| public class CombineCombinator extends AggregateFunction | ||
| implements ExplicitlyCastableSignature, AlwaysNotNullable, Combinator, RollUpTrait { |
There was a problem hiding this comment.
[P1] Preserve the non-null empty state across scalar subqueries
This aggregate promises a non-null state through AlwaysNotNullable, but scalar-subquery nullability adjustment and correlated empty-input repair only recognize NotNullableAggregateFunction. As a result, (select avg_combine(v) from r) is exposed as nullable, and feeding that state to _merge or _union reaches BE as Nullable(AggState), which AggFnEvaluator::prepare() rejects. Correlated unnesting can also substitute SQL NULL for an unmatched key instead of the serialized empty state. Please integrate _combine with the scalar-aggregate empty-input contract (or prevent these rewrites/uses) and cover uncorrelated plus unmatched-correlated cases.
There was a problem hiding this comment.
Acknowledged and deferred to a follow-up PR, as discussed in #66942 (comment). This is shared by _union and _combine: correlated scalar-subquery rewriting needs a typed empty AggState expression and a common empty-input result contract. We will fix both combinators uniformly rather than special-casing _combine in this PR.
| } | ||
|
|
||
| @Override | ||
| protected List<DataType> intermediateTypes() { |
There was a problem hiding this comment.
[P1] Delegate the nested aggregate's phase support
The wrapper delegates intermediateTypes() here but inherits the base supportAggregatePhase(), which returns true for every phase. Consequently orthogonal_bitmap_expr_calculate[_count]_combine(...) appears eligible for a forced agg_phase=1 INPUT_TO_RESULT plan even though both nested aggregates explicitly support only AggregatePhase.TWO. Please delegate this policy to nested and add a phase-selection test for a two-phase-only aggregate; the supported two-phase serialization path also needs the separate state-preservation fix.
There was a problem hiding this comment.
Fixed. CombineCombinator.supportAggregatePhase now delegates to the nested aggregate function, so aggregates that disallow a phase remain disallowed after wrapping. The orthogonal bitmap functions are additionally rejected from AggState combinators, and the delegation behavior is covered by unit test.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
|
Follow-up issue: correlated scalar subqueries containing Both combinators produce an A complete fix should introduce a shared empty-input-result contract and a typed empty |
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: The aggregate combine path admitted orthogonal bitmap aggregates whose states are not safe for AggState serialization, did not propagate QueryContext to AI aggregate combinators, allowed unsupported synchronous materialized view definitions, and ignored nested aggregate phase restrictions. Mark orthogonal bitmap aggregates as unsupported by AggState combinators, propagate AI query context by function-name prefix, reject combine functions in synchronous materialized view analysis, and delegate aggregate phase support to the nested function. ### Release note Harden aggregate state combine validation and execution setup. ### Check List (For Author) - Test: - Unit Test: CombineCombinatorTest - Regression test: Added synchronous materialized view rejection coverage to test_agg_state_avg; not run locally because no worktree-local cluster was started - BE Unit Test: Not run because run-be-ut.sh requires updating the datasketches-cpp submodule before compilation - Behavior changed: Yes. Orthogonal bitmap AggState combinators and synchronous materialized views using _combine are rejected during analysis. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: Aggregate combine functions reused nested states that were not constructed with window semantics, and dynamic combinator resolution could wrap scalar function builders and later fail with an internal class cast. Reject combine functions during window analysis and only advertise AggState combinators for aggregate function builders. ### Release note Reject aggregate combine window functions and scalar functions with AggState combinator suffixes during analysis. ### Check List (For Author) - Test: - Unit Test: CombineCombinatorTest - Unit Test: CheckAndStandardizeWindowFunctionTest - Behavior changed: Yes. Aggregate combine window functions and scalar functions with AggState combinator suffixes now return analysis errors. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: AI_AGG normalizes two-argument calls by adding a default resource, while AggState-producing combinators preserve the original arguments. This can create mismatched AggState signatures and invalid backend execution. Reject ai_agg_state and ai_agg_combine during function resolution until canonical AggState support is implemented. ### Release note AI_AGG no longer supports the _state and _combine combinators. ### Check List (For Author) - Test: Unit Test - CombineCombinatorTest - Behavior changed: Yes. ai_agg_state and ai_agg_combine now fail analysis. - Does this need documentation: No
|
/review |
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: FE Checkstyle rejected the imports in CombineCombinatorTest because AIAgg was not in the required custom lexicographical order. Sort the imports according to the repository rule. ### Release note None ### Check List (For Author) - Test: Manual test - mvn -T 48 clean checkstyle:check - Behavior changed: No - Does this need documentation: No
HappenLee
changed the title
There was a problem hiding this comment.
Automated review result: requesting changes on head 4c317b6d04fe91f3bed5bb32cab142426f16d5c3 after two converged review rounds. One new inline P1 is attached.
Blocking finding:
_combinehas no execution-version or backend-capability fence for an old BE that remains query-available as a cloud smooth-upgrade source. The new FE metadata reaches the target-base AGG_STATE branch and fails during evaluator preparation.
Critical checkpoint conclusions:
- Goal and correctness: the current-FE/current-BE raw-input-to-serialized-state path is coherent, including grouped/non-grouped finalization, partial/streaming aggregation, nullable/all-null states, and merge/union consumption. The mixed-version dispatch path is the surviving blocker.
- Scope and user focus: all 27 authoritative changed files plus relevant binding, scheduling, cast, MV/window, and BE aggregation consumers were swept. No extra user-provided focus was supplied, and no unrelated source change was found.
- Concurrency, lifecycle, and memory: the wrapper adds no shared mutable state, threads, locks, or ownership cycle; nested create/reset/destroy, size/alignment, context, blockability, COW ownership, and append-safe result serialization were traced without a distinct defect.
- Compatibility and FE/BE metadata: function name, raw parameter types/nullability, AggState return metadata, and binary type agree on current binaries. Base and head both advertise BE execution version 13, while cloud smooth-upgrade worker selection can retain an old source BE; that missing capability boundary is the inline issue.
- Parallel paths and conditions: DISTINCT, zero-argument, ordered/window, AI, orthogonal bitmap, foreach, synchronous/asynchronous MV, aliases, CTE/subquery, UNION/CASE common types, explicit casts, and sinks were checked. Existing scalar-subquery and Decimal256 limitations remain on their live threads and were not duplicated.
- Error handling and nullability: current-branch type/name/arity mismatches return contextual Status errors, invariants fail loudly, const/nullable arguments are materialized or delegated consistently, and no swallowed error survived. The old-BE prepare failure should instead be prevented by planning/scheduling.
- Tests and expected results: the new BE, FE, and regression coverage and every expected AVG result were inspected. Mixed-version scheduling is not covered. No build or test command was run because the authoritative review prompt prohibits it.
- Observability, persistence, transactions, writes, and configuration: no new metric/logging requirement, persisted format, EditLog, transaction protocol, storage-write path, or configuration default is introduced.
- Performance: the intended avoidance of per-row state materialization is present; current result/partial paths did not reveal a separate CPU, copy, or peak-memory blocker.
- Final falsification: a proposed UNION exact-policy issue was rejected before submission because the later non-loose CheckCast rejects the non-identical AggState cast before translation. A cross-name merge/union validation mismatch was also dismissed as pre-existing.
Review status: complete and converged after Round 2. BE, FE, and risk-focused passes each ended with NO_NEW_VALUABLE_FINDINGS; every other candidate was dismissed with code evidence or duplicate-fenced.
| Function aggregateFunction = functionCallExpr.getFn(); | ||
| aggregateFunction.setName(new FunctionName(combinator.getName())); | ||
| aggregateFunction.setArgs(arguments.stream().map(Expr::getType).collect(Collectors.toList())); | ||
| aggregateFunction.setBinaryType(Function.BinaryType.AGG_STATE); |
There was a problem hiding this comment.
[P1] Fence _combine during cloud smooth upgrades. This translator sends raw arguments with binary_type=AGG_STATE, but the target-base BE treats every AGG_STATE function as a one-argument merge/union consumer. Base and head both advertise execution version 13, and cloud worker selection can still choose an isSmoothUpgradeSrc BE because Backend.isQueryAvailable() does not exclude it. avg_combine(INT) then fails old-BE prepare (input type must be agg_state; multi-argument combines fail arity). Please add an execution-version/backend-capability or smooth-upgrade-source fence and cover mixed-version scheduling.
There was a problem hiding this comment.
Thanks for raising this. This failure path requires a new FE that can plan and emit _combine to coexist with an old, query-eligible BE.
Under the supported Doris upgrade contract, all BEs are upgraded before any FE is upgraded:
- During the mixed-BE phase, the FE is still old and cannot resolve or emit
_combine. - During the FE rolling-upgrade phase, all BEs have already been upgraded and support
AggregateStateCombine.
Therefore, isSmoothUpgradeSrc() being query-available by itself does not make this path reachable. The documented Cloud upgrade procedure explicitly requires confirming that all BE instances have been upgraded before upgrading FE:
https://doris.apache.org/docs/4.x/compute-storage-decoupled/upgrade/
The general upgrade procedure states the same BE-before-FE ordering:
https://doris.apache.org/docs/dev/admin-manual/cluster-management/upgrade/
Keeping the same be_exec_version is also expected here because this change does not alter the serialized block format; compatibility is provided by the BE-first rollout order.
Unless there is a supported Cloud rollout workflow in which a new FE is deployed before every old smooth-upgrade source BE in every compute group has been retired, this mixed-version path is outside the supported upgrade sequence and should not require a runtime fence. Please identify such a supported workflow if one exists; otherwise I believe this finding can be resolved.
|
run buildall |
TPC-H: Total hot run time: 16881 ms |
TPC-DS: Total hot run time: 82362 ms |
ClickBench: Total hot run time: 14.8 s |
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Existing <aggregate>_state plus <aggregate>_union constructs and serializes one aggregate state for every input row before merging those states. Add <aggregate>_combine to aggregate raw inputs directly with the nested aggregate function and emit one compatible AggState, preserving partial aggregation and avoiding per-row state materialization.
### Release note
Add the experimental <aggregate>_combine(...) combinator for directly producing reusable aggregate states.
### Check List (For Author)
- Test:
- Regression test: test_agg_state_avg
- Unit Test: AggregateStateCombineTest and CombineCombinatorTest
- Behavior changed: Yes. Adds a new aggregate-state combinator.
- Does this need documentation: No. The function is covered by the existing experimental AggState feature.
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: The aggregate combine path admitted orthogonal bitmap aggregates whose states are not safe for AggState serialization, did not propagate QueryContext to AI aggregate combinators, allowed unsupported synchronous materialized view definitions, and ignored nested aggregate phase restrictions. Mark orthogonal bitmap aggregates as unsupported by AggState combinators, propagate AI query context by function-name prefix, reject combine functions in synchronous materialized view analysis, and delegate aggregate phase support to the nested function. ### Release note Harden aggregate state combine validation and execution setup. ### Check List (For Author) - Test: - Unit Test: CombineCombinatorTest - Regression test: Added synchronous materialized view rejection coverage to test_agg_state_avg; not run locally because no worktree-local cluster was started - BE Unit Test: Not run because run-be-ut.sh requires updating the datasketches-cpp submodule before compilation - Behavior changed: Yes. Orthogonal bitmap AggState combinators and synchronous materialized views using _combine are rejected during analysis. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: Aggregate combine functions reused nested states that were not constructed with window semantics, and dynamic combinator resolution could wrap scalar function builders and later fail with an internal class cast. Reject combine functions during window analysis and only advertise AggState combinators for aggregate function builders. ### Release note Reject aggregate combine window functions and scalar functions with AggState combinator suffixes during analysis. ### Check List (For Author) - Test: - Unit Test: CombineCombinatorTest - Unit Test: CheckAndStandardizeWindowFunctionTest - Behavior changed: Yes. Aggregate combine window functions and scalar functions with AggState combinator suffixes now return analysis errors. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: AI_AGG normalizes two-argument calls by adding a default resource, while AggState-producing combinators preserve the original arguments. This can create mismatched AggState signatures and invalid backend execution. Reject ai_agg_state and ai_agg_combine during function resolution until canonical AggState support is implemented. ### Release note AI_AGG no longer supports the _state and _combine combinators. ### Check List (For Author) - Test: Unit Test - CombineCombinatorTest - Behavior changed: Yes. ai_agg_state and ai_agg_combine now fail analysis. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: FE Checkstyle rejected the imports in CombineCombinatorTest because AIAgg was not in the required custom lexicographical order. Sort the imports according to the repository rule. ### Release note None ### Check List (For Author) - Test: Manual test - mvn -T 48 clean checkstyle:check - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: AggState capability checks were inconsistent across SQL binding and synchronous materialized view rewriting. AI aggregates could still bind merge and union wrappers, orthogonal aggregate foreach support was accidentally blocked, combine accepted DISTINCT during overload matching and failed later with an unchecked exception, and synchronous materialized views bypassed the function-builder capability check. Use the common NotSupportAggState marker for AI and orthogonal aggregates, preserve foreach support, reject DISTINCT combine calls during binding, and enforce the capability in StateCombinator construction so all state creation paths share the same rule. ### Release note Unsupported AggState wrappers are rejected during FE analysis while orthogonal foreach remains available. ### Check List (For Author) - Test: Unit Test and regression test coverage - Unit Test: CombineCombinatorTest - Regression test: Added synchronous materialized view rejection coverage (not run locally) - FE Checkstyle: Passed - Behavior changed: Yes. Unsupported AI AggState wrappers, DISTINCT combine calls, and unsupported synchronous materialized view state creation are rejected during analysis. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: Restore exact QueryContext dispatch for ai_agg, classify dynamically synthesized _combine functions as aggregate functions before argument binding while preserving UDF precedence, and leave incompatible CombineCombinator AggState casts to the standard cast checker instead of rewriting mismatched child metadata. ### Release note Fix aggregate-state combine analysis and reject incompatible combine AggState casts with a normal analysis error. ### Check List (For Author) - Test: Unit Test - CombineCombinatorTest - FillUpMissingSlotsTest#testCombineCombinatorBindsArgumentsInAggregateInputScope - BE clang-format check - FE checkstyle - Behavior changed: Yes. Dynamic _combine calls bind in aggregate input scope, incompatible combine AggState casts are rejected, and QueryContext dispatch only applies to ai_agg. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: AggregateStateCombine delegated every output row to the nested serialize_without_key_to_column implementation. Nullable count resizes that destination to one row, so grouped combine output overwrote earlier states. Serialize grouped places through the nested batch interface and append the resulting states to the destination column. Also serialize a single result into an empty temporary column before appending it, preserving existing rows. ### Release note Fix grouped _combine output for aggregate states whose single-state serializer does not append. ### Check List (For Author) - Test: Unit Test - AggregateStateCombineTest.* - BE format, header hygiene, and clang-tidy checks - Behavior changed: Yes, grouped _combine now preserves one state per group - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: `_combine` states could enter the existing loose AggState coercion path for explicit CAST and INSERT sinks. After aggregate extraction, the cast could no longer retarget the raw producer, so analysis accepted the conversion initially and rejected it later with an inconsistent error. Mark `_combine` output as exact-match-only during planning, preserve that policy across type conversion, and reject subtype or argument-nullability mismatches with an actionable analysis error. Existing `_state` loose coercion remains unchanged. ### Release note Reject non-exact AggState CAST and INSERT coercion for `_combine` with a clear analysis error. ### Check List (For Author) - Test: - Unit Test: Added exact subtype/nullability checks, explicit CAST coverage, and a compatibility assertion for existing `_state` coercion. Local execution was blocked by an unrelated `apache/master` testCompile error in `InsertIntoTableCommandTableStreamTest`; FE CheckStyle and main-source compilation passed. - Regression test: Added rejected non-exact CAST/INSERT cases and an accepted exact INSERT case; not run locally because the rebased master testCompile error prevented producing a complete FE build. - Behavior changed: Yes. `_combine` output now accepts only exact AggState matches. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: FunctionRegistry classified unknown `_combine` names as aggregate functions from the nested function name before argument-aware resolution. This diverged from the existing `_union` behavior and could bind an exact scalar UDF such as a two-argument `avg_combine` in the wrong aggregate scope. BE combine finalization also serialized into a temporary column and then deep-copied variable-width states even when the destination was empty. Remove the name-only `_combine` classification fallback, preserve argument-aware combinator and exact UDF resolution, and serialize directly into empty single-row and vector destinations while retaining the append-safe fallback for nonempty destinations. ### Release note Resolve exact scalar UDFs ending in `_combine` correctly and avoid redundant copies when finalizing combine states into empty output columns. ### Check List (For Author) - Test: - Unit Test: BE AggregateStateCombineTest passed all 4 tests, including an 8-by-4096-row grouped array-state combine/merge case. FE CombineCombinatorTest passed. FillUpMissingSlotsTest compiled, but its embedded FE setup could not run because the host TCP ephemeral port pool was exhausted before either target method started. - Behavior changed: Yes. Unknown `_combine` names now follow the historical `_union` pre-binding behavior, and empty combine output columns use direct serialization. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: None Related PR: apache#66942 Problem Summary: Dynamic aggregate-state combinators are synthesized from built-in aggregate names, but `_combine` pre-binding and final resolution diverged while attempting to preserve same-named UDF fallback. This could bind valid `avg_combine` arguments in aggregate-output scope before resolving the call as an aggregate. Treat all dynamically synthesized AggState combinator names as reserved built-in names, reject UDF creation with those names, remove the same-name UDF compatibility path, and verify HAVING and ORDER BY bind the raw input slot. ### Release note Reserve dynamically synthesized aggregate-state combinator names for built-in functions. ### Check List (For Author) - Test: Unit Test - CombineCombinatorTest - FillUpMissingSlotsTest - CreateFunctionTest - Behavior changed: Yes. Dynamic aggregate-state combinator names can no longer be created as UDFs. - Does this need documentation: No
4c317b6 to
4052cec
Compare
|
run buildall |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16780 ms |
TPC-DS: Total hot run time: 81599 ms |
ClickBench: Total hot run time: 14.44 s |
3 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Existing
<aggregate>_stateplus<aggregate>_unionconstructs and serializes one aggregate state for every input row before merging those states. For queries with tens of millions of raw rows, this per-row state materialization can erase much of the pre-aggregation benefit.This PR adds a generic
<aggregate>_combine(...)combinator. It feeds raw arguments directly into the nested aggregate function and emits one compatibleAggStateper group. For example,avg_combine(x)is aggregation-equivalent toavg_union(avg_state(x)), while avoiding a serialized AVG state for every source row. The resulting state remains consumable by the existing_unionand_mergecombinators.The BE wrapper preserves the nested aggregate state layout and delegates raw accumulation, partial serialization, merge/deserialization, nullable handling, and fixed-length serialized column creation. Nereids resolves and translates the generic combinator and supports MV roll-up through the matching
_unionfunction. Zero-argument aggregates, DISTINCT, and aggregate ORDER BY expressions are rejected for now.Dynamically synthesized
<aggregate>_state,<aggregate>_merge,<aggregate>_union,<aggregate>_combine, and<aggregate>_foreachnames are reserved for built-in combinators. New UDFs cannot use these names, and unqualified calls resolve to the built-in combinator without same-named UDF fallback. Existing UDFs can still be addressed with an explicit database qualifier.Release note
Add the experimental
<aggregate>_combine(...)combinator for directly producing reusable aggregate states. Dynamically synthesized aggregate-state combinator names are reserved for built-in functions.Check List (For Author)
Regression coverage:
test_agg_state_avg, including nullable input, all-null input, grouped partial aggregation, and compatibility withavg_union(avg_state(...)).Unit coverage:
AggregateStateCombineTest,CombineCombinatorTest,FillUpMissingSlotsTest, andCreateFunctionTest. Full BE and FE builds also passed; clang-tidy reported no warnings for the changed BE files.Behavior changed:
Does this need documentation?