HappenLee commented on code in PR #66942:
URL: https://github.com/apache/doris/pull/66942#discussion_r3842546494
##########
be/src/exprs/vectorized_agg_fn.cpp:
##########
@@ -256,7 +292,7 @@ Status AggFnEvaluator::prepare(RuntimeState* state, const
RowDescriptor& desc,
_sort_description, state);
}
- if (_fn.name.function_name == "ai_agg") {
+ if (_fn.name.function_name.starts_with("ai_agg")) {
Review Comment:
Fixed in 888aab361e8. AIAgg now uses the same NotSupportAggState capability
marker as the orthogonal aggregate family. FE binding rejects _state, _combine,
_merge, and _union for AI aggregates, so these unsupported wrappers cannot
reach BE and no wrapper-side QueryContext forwarding is required. The narrower
NotSupportAggStateCreation marker has been removed. Unit coverage verifies all
four suffixes.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/AggCombinerFunctionBuilder.java:
##########
@@ -65,7 +70,21 @@ public Class<? extends BoundFunction> functionClass() {
@Override
public boolean canApply(List<?> arguments) {
- if (combinatorSuffix.equalsIgnoreCase(STATE) ||
combinatorSuffix.equalsIgnoreCase(FOREACH)) {
+ if
(!AggregateFunction.class.isAssignableFrom(nestedBuilder.functionClass())) {
+ return false;
+ }
+ if
(NotSupportAggState.class.isAssignableFrom(nestedBuilder.functionClass())) {
Review Comment:
Fixed in 888aab361e8. NotSupportAggState now rejects only
AggState-producing/consuming suffixes and explicitly preserves _foreach, which
is not an AggState wrapper. A positive unit test verifies that
orthogonal_bitmap_union_count_foreach is still resolved and built successfully.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/AggCombinerFunctionBuilder.java:
##########
@@ -139,6 +158,12 @@ public Pair<BoundFunction, AggregateFunction> build(String
name, List<?> argumen
arguments = arguments.subList(1, arguments.size());
}
return Pair.of(new StateCombinator((List<Expression>) arguments,
nestedFunction), nestedFunction);
+ } else if (combinatorSuffix.equalsIgnoreCase(COMBINE)) {
+ AggregateFunction nestedFunction = buildState(nestedName,
arguments);
+ if (!arguments.isEmpty() && arguments.get(0) instanceof Boolean &&
(Boolean) arguments.get(0)) {
+ throw new IllegalStateException(name + " doesn't support
DISTINCT");
Review Comment:
Fixed in 888aab361e8. The combine builder now rejects the prepended DISTINCT
marker in canApply before invoking the nested builder, instead of accepting the
overload and failing later with an unchecked IllegalStateException. The test
now analyzes the real SQL select avg_combine(distinct 1) and verifies an
AnalysisException.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreateMaterializedViewCommand.java:
##########
@@ -659,6 +660,10 @@ private void checkNoNondeterministicFunctionOrUnnest(Plan
plan) {
}
private void validateAggFunnction(AggregateFunction aggregateFunction)
{
+ if (aggregateFunction instanceof CombineCombinator) {
Review Comment:
Fixed in 888aab361e8. The NotSupportAggState check is now also enforced at
the common StateCombinator construction boundary. Therefore direct
StateCombinator.create calls from synchronous MV rewriting cannot bypass the
function-builder capability check. Unit tests cover direct AI/orthogonal state
construction, and a synchronous MV negative regression case was added.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/CombineCombinator.java:
##########
@@ -0,0 +1,164 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.trees.expressions.functions.combinator;
+
+import org.apache.doris.catalog.BuiltinAggregateFunctions;
+import org.apache.doris.catalog.Env;
+import org.apache.doris.catalog.FunctionRegistry;
+import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.common.Pair;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.OrderExpression;
+import
org.apache.doris.nereids.trees.expressions.functions.AggCombinerFunctionBuilder;
+import org.apache.doris.nereids.trees.expressions.functions.AlwaysNotNullable;
+import org.apache.doris.nereids.trees.expressions.functions.BoundFunction;
+import
org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature;
+import org.apache.doris.nereids.trees.expressions.functions.ExpressionTrait;
+import org.apache.doris.nereids.trees.expressions.functions.Function;
+import org.apache.doris.nereids.trees.expressions.functions.FunctionBuilder;
+import
org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction;
+import
org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunctionParams;
+import org.apache.doris.nereids.trees.expressions.functions.agg.AggregatePhase;
+import org.apache.doris.nereids.trees.expressions.functions.agg.RollUpTrait;
+import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
+import org.apache.doris.nereids.types.AggStateType;
+import org.apache.doris.nereids.types.DataType;
+
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * Aggregate inputs into the nested function's serialized state.
+ */
+public class CombineCombinator extends AggregateFunction
+ implements ExplicitlyCastableSignature, AlwaysNotNullable, Combinator,
RollUpTrait {
+
+ private final AggregateFunction nested;
+ private final AggStateType returnType;
+
+ /** Constructor of CombineCombinator. */
+ public CombineCombinator(List<Expression> arguments, AggregateFunction
nested) {
+ super(nested.getName() + AggCombinerFunctionBuilder.COMBINE_SUFFIX,
arguments);
+ checkArguments(arguments, nested);
+ this.nested = Objects.requireNonNull(nested, "nested can not be null");
+ this.returnType = createReturnType(arguments, nested);
+ }
+
+ private CombineCombinator(AggregateFunctionParams functionParams,
AggregateFunction nested) {
+ super(functionParams);
+ checkArguments(functionParams.arguments, nested);
+ this.nested = Objects.requireNonNull(nested, "nested can not be null");
+ this.returnType = createReturnType(functionParams.arguments, nested);
+ }
+
+ private static void checkArguments(List<Expression> arguments,
AggregateFunction nested) {
+ if (arguments.isEmpty()) {
+ throw new AnalysisException(String.format(
+ "%s_combine requires at least one argument",
nested.getName()));
+ }
+ for (Expression argument : arguments) {
+ if (argument instanceof OrderExpression) {
+ throw new AnalysisException(String.format(
+ "%s_combine doesn't support order by expression",
nested.getName()));
+ }
+ }
+ }
+
+ private static AggStateType createReturnType(List<Expression> arguments,
AggregateFunction nested) {
+ return new AggStateType(nested.getName(),
Review Comment:
Acknowledged. The Decimal256 AVG metadata inconsistency is intentionally
deferred and is not changed in this PR. We will address it separately after
agreeing on the expected AggState metadata behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
