github-actions[bot] commented on code in PR #68312: URL: https://github.com/apache/doris/pull/68312#discussion_r4059519364
########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/FinalizeCombinator.java: ########## @@ -0,0 +1,98 @@ +// 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.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.functions.AggCombinerFunctionBuilder; +import org.apache.doris.nereids.trees.expressions.functions.ComputeNullable; +import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; +import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ScalarFunction; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ScalarFunctionParams; +import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.AggStateType; + +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** Finalize each serialized aggregate state without aggregating rows. */ +public class FinalizeCombinator extends ScalarFunction + implements UnaryExpression, ExplicitlyCastableSignature, ComputeNullable, Combinator { Review 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. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/FinalizeCombinator.java: ########## @@ -0,0 +1,98 @@ +// 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.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.functions.AggCombinerFunctionBuilder; +import org.apache.doris.nereids.trees.expressions.functions.ComputeNullable; +import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; +import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ScalarFunction; +import org.apache.doris.nereids.trees.expressions.functions.scalar.ScalarFunctionParams; +import org.apache.doris.nereids.trees.expressions.shape.UnaryExpression; +import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor; +import org.apache.doris.nereids.types.AggStateType; + +import com.google.common.collect.ImmutableList; + +import java.util.List; + +/** Finalize each serialized aggregate state without aggregating rows. */ +public class FinalizeCombinator extends ScalarFunction + implements UnaryExpression, ExplicitlyCastableSignature, ComputeNullable, Combinator { + + private final AggregateFunction nested; + + public FinalizeCombinator(List<Expression> arguments, AggregateFunction nested) { + super(nested.getName() + AggCombinerFunctionBuilder.FINALIZE_SUFFIX, arguments); + this.nested = nested; + checkStateFunction(); + } + + private FinalizeCombinator(ScalarFunctionParams functionParams, AggregateFunction nested) { + super(functionParams); + this.nested = nested; + checkStateFunction(); + } + + private void checkStateFunction() { + AggStateType inputType = (AggStateType) getArgument(0).getDataType(); + // 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())) { Review 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`. -- 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]
