github-actions[bot] commented on code in PR #65847:
URL: https://github.com/apache/doris/pull/65847#discussion_r3651919995
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/MergeCombinator.java:
##########
@@ -117,6 +117,12 @@ public boolean canRollUp() {
@Override
public void checkLegalityBeforeTypeCoercion() {
- nested.checkLegalityBeforeTypeCoercion();
+ // A directly nested state retains the original value expressions, so
their legality
+ // checks are still meaningful. After a subquery or stored-column
boundary, the nested
+ // function is rebuilt with mocked slots from AggStateType; replaying
value-expression
+ // checks there would reject valid states whose constants are already
serialized.
+ if (getArgument(0) instanceof StateCombinator) {
Review Comment:
**[P2] Keep stored-state type checks when skipping literal checks.** This
guard also suppresses nested legality checks that depend only on the stored
subtype. For example, DDL accepts `AGG_STATE<max(BITMAP)>` after validating
only that `max` is registered; `max_merge(s)` rebuilds `Max` with a mocked
BITMAP slot, its custom signature accepts that type, and this branch skips
`Max.checkLegalityBeforeTypeCoercion`, even though direct
`max(bitmap)`/`max_state(bitmap)` reject metric-only types. Before this change,
the unconditional delegation rejected the same plan. Please separate
value-expression checks that cannot be replayed from state-safe type/shape
checks (or validate the nested signature when `AggStateType` is declared), and
cover stored `_merge`/`_union` with an illegal subtype.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApproxArray.java:
##########
@@ -0,0 +1,127 @@
+// 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.agg;
+
+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.ExplicitlyCastableSignature;
+import org.apache.doris.nereids.trees.expressions.literal.ArrayLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.Literal;
+import org.apache.doris.nereids.trees.expressions.literal.NullLiteral;
+import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
+import org.apache.doris.nereids.types.ArrayType;
+import org.apache.doris.nereids.types.DoubleType;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * AggregateFunction 'percentile_approx_array'.
+ */
+public class PercentileApproxArray extends NotNullableAggregateFunction
+ implements ExplicitlyCastableSignature {
+
+ public static final List<FunctionSignature> SIGNATURES = ImmutableList.of(
+ FunctionSignature.ret(ArrayType.of(DoubleType.INSTANCE))
+ .args(DoubleType.INSTANCE,
ArrayType.of(DoubleType.INSTANCE)),
+ FunctionSignature.ret(ArrayType.of(DoubleType.INSTANCE))
+ .args(DoubleType.INSTANCE,
ArrayType.of(DoubleType.INSTANCE), DoubleType.INSTANCE)
+ );
+
+ public PercentileApproxArray(Expression arg0, Expression arg1) {
+ this(false, arg0, arg1);
+ }
+
+ public PercentileApproxArray(boolean distinct, Expression arg0, Expression
arg1) {
+ super("percentile_approx_array", distinct, arg0, arg1);
+ }
+
+ public PercentileApproxArray(Expression arg0, Expression arg1, Expression
arg2) {
+ this(false, arg0, arg1, arg2);
+ }
+
+ public PercentileApproxArray(boolean distinct, Expression arg0, Expression
arg1,
+ Expression arg2) {
+ super("percentile_approx_array", distinct, arg0, arg1, arg2);
+ }
+
+ /** constructor for withChildren and reuse signature */
+ private PercentileApproxArray(AggregateFunctionParams functionParams) {
+ super(functionParams);
+ }
+
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ if (!getArgument(1).isConstant()) {
+ throw new AnalysisException(
+ "percentile_approx_array requires second parameter must be
a constant : " + this.toSql());
+ }
+ if (arity() == 3 && !getArgument(2).isConstant()) {
+ throw new AnalysisException(
+ "percentile_approx_array requires the third parameter must
be a constant : " + this.toSql());
+ }
+ }
+
+ @Override
+ public void checkLegalityAfterRewrite() {
+ checkLegalityBeforeTypeCoercion();
+ Expression quantiles = getArgument(1);
+ if (!(quantiles instanceof ArrayLiteral)) {
+ return;
+ }
+ for (Expression item : ((ArrayLiteral) quantiles).getValue()) {
+ if (item instanceof NullLiteral) {
+ throw new AnalysisException(
+ "percentile_approx_array quantile should not be null :
" + this.toSql());
+ }
+ if (!(item instanceof Literal) ||
!item.getDataType().isNumericType()) {
+ continue;
+ }
+ double value = ((Literal) item).getDouble();
+ if (!Double.isFinite(value) || value < 0.0 || value > 1.0) {
+ throw new AnalysisException("percentile_approx_array quantile
must be in [0, 1], but got "
+ + value + ": " + this.toSql());
+ }
+ }
+ }
+
+ @Override
+ public PercentileApproxArray withDistinctAndChildren(boolean distinct,
List<Expression> children) {
+ Preconditions.checkArgument(children.size() == 2 || children.size() ==
3);
+ return new PercentileApproxArray(getFunctionParams(distinct,
children));
+ }
+
+ @Override
+ public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
+ return visitor.visitPercentileApproxArray(this, context);
+ }
+
+ @Override
+ public List<FunctionSignature> getSignatures() {
+ return SIGNATURES;
+ }
+
+ @Override
+ public Expression resultForEmptyInput() {
Review Comment:
**[P2] Preserve `[]` through correlated `_merge` subqueries.** A reduced
trigger is `Project((scalar subquery)) ->
Aggregate(percentile_approx_array_merge(s)) -> Filter(inner.k = outer.k)`.
After decorrelation, an unmatched outer key is supplied by a left join.
`SubqueryToApply.addNvlForScalarSubqueryOutput` inserts the aggregate's empty
default only when the alias child is itself a `NotNullableAggregateFunction`;
`MergeCombinator` is a plain `AggregateFunction`, so the unmatched slot remains
`NULL` even though its nested aggregate is non-nullable and this method defines
empty input as `[]`. Please expose the nested empty-result contract to the
scalar-subquery rewrite and add an unmatched stored-state regression.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/BuiltinAggregateFunctions.java:
##########
@@ -190,6 +191,7 @@ private BuiltinAggregateFunctions() {
agg(Percentile.class, "percentile", "percentile_cont"),
agg(PercentileReservoir.class, "percentile_reservoir"),
agg(PercentileApprox.class, "percentile_approx"),
+ agg(PercentileApproxArray.class, "percentile_approx_array"),
Review Comment:
**[P2] Preserve empty `_union` states through correlated subqueries.** This
registration also synthesizes `percentile_approx_array_union`. For `SELECT
(SELECT percentile_approx_array_union(s) FROM inner_t WHERE inner_t.k =
outer_t.k) FROM outer_t`, decorrelation supplies unmatched keys from the NULL
side of a left join. The rewrite installs an empty default only for
`NotNullableAggregateFunction`, but `UnionCombinator` is instead an
`AlwaysNotNullable` plain aggregate; BE's direct zero-row union serializes a
non-null empty state. Thus unmatched keys return NULL rather than the direct
aggregate's empty `AGG_STATE`. Please preserve the union empty-state contract
during scalar-subquery decorrelation and cover an unmatched key.
--
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]
