github-actions[bot] commented on code in PR #64624:
URL: https://github.com/apache/doris/pull/64624#discussion_r3662638601
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileApproxWeighted.java:
##########
@@ -89,19 +90,36 @@ private
PercentileApproxWeighted(NullableAggregateFunctionParams functionParams)
@Override
public void checkLegalityBeforeTypeCoercion() {
- if (!getArgument(2).isConstant()) {
+ Expression quantile = getArgument(2);
+ if (!quantile.isConstant()) {
throw new AnalysisException(
"percentile_approx_weighted requires the third parameter
must be a constant : " + this.toSql());
}
if (arity() == 4) {
- if (!getArgument(3).isConstant()) {
+ Expression compressionArg = getArgument(3);
+ if (!compressionArg.isConstant()) {
throw new AnalysisException(
"percentile_approx_weighted requires the fourth
parameter must be a constant : "
+ this.toSql());
}
}
}
+ @Override
+ public void checkLegalityAfterRewrite() {
+ checkLegalityBeforeTypeCoercion();
Review Comment:
Re-running `checkLegalityBeforeTypeCoercion()` here rejects valid
grouping-sets plans after `NormalizeRepeat` has deliberately represented the
already-validated constant as a slot. For example:
```text
Aggregate(percentile_approx_weighted(v_slot, w_slot, q_slot))
Repeat(..., q_slot)
Project(..., 0.5 AS q_slot)
```
`percentile_approx_weighted(v, w, 0.5) ... GROUP BY GROUPING SETS (...)`
passes the initial legality check, but this post-rewrite call sees
`q_slot.isConstant() == false` and fails analysis. Please keep this method to
post-fold literal range validation, as the other percentile implementations do,
and add a positive grouping-sets case for the weighted function.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java:
##########
@@ -98,7 +98,7 @@ public static StateCombinator create(AggregateFunction
nested) {
@Override
public StateCombinator withChildren(List<Expression> children) {
- return new StateCombinator(getFunctionParams(children), nested);
+ return new StateCombinator(getFunctionParams(children),
nested.withChildren(children));
Review Comment:
The two-argument `ai_agg_state` overload has fewer wrapper children than its
nested aggregate. `AggCombinerFunctionBuilder` constructs the wrapper from the
two SQL arguments, but `AIAgg(arg0, arg1)` injects the default resource and
stores three nested children. If coercion rewrites a visible child, for example:
```text
Project(ai_agg_state(CAST(int_col AS STRING), 'task'))
Scan(int_col)
```
this method calls `AIAgg.withChildren()` with two children, which violates
its three-child precondition and rejects the otherwise valid default-resource
call during analysis. Please preserve the injected resource while synchronizing
the visible children (or construct the wrapper from the nested aggregate's full
argument list), and add a coercion test for the two-argument overload.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/AIAgg.java:
##########
@@ -52,17 +52,31 @@ public class AIAgg extends NullableAggregateFunction
);
/**
- * constructor with 1 argument.
+ * Constructor with default resource.
*/
public AIAgg(Expression arg0, Expression arg1) {
this(new StringLiteral(getResourceName()), arg0, arg1);
}
/**
- * constructor with 2 argument.
+ * Constructor with default resource and distinct.
+ */
+ public AIAgg(boolean distinct, Expression arg0, Expression arg1) {
+ this(distinct, new StringLiteral(getResourceName()), arg0, arg1);
+ }
+
+ /**
+ * Constructor with explicit resource.
*/
public AIAgg(Expression arg0, Expression arg1, Expression arg2) {
- super("ai_agg", false, false, arg0, arg1, arg2);
+ this(false, arg0, arg1, arg2);
+ }
+
+ /**
+ * Constructor with explicit resource and distinct.
+ */
+ public AIAgg(boolean distinct, Expression arg0, Expression arg1,
Expression arg2) {
+ super("ai_agg", distinct, false, arg0, arg1, arg2);
Review Comment:
The newly enabled DISTINCT form is rejected when it appears above `Repeat`,
because normalization replaces the required literal resource and task with
slots:
```text
Aggregate(AI_AGG(DISTINCT resource_slot, text_slot, task_slot))
Repeat(...)
Project('r' AS resource_slot, text_col AS text_slot, 'summarize' AS
task_slot, ...)
```
The original call passes `AIAgg`'s literal/resource checks, but the final
legality pass sees `resource_slot` and `task_slot` and rejects the plan before
DISTINCT splitting. Unlike the weighted-percentile case, simply removing a
repeated constantness check is not enough because `AIAgg` still needs the
authoritative resource/task literals. Please preserve those arguments through
`NormalizeRepeat` (or retain their original literal metadata) and add
explicit/default-resource DISTINCT grouping-sets tests.
--
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]
