github-actions[bot] commented on code in PR #65664:
URL: https://github.com/apache/doris/pull/65664#discussion_r3601010409
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/CollectList.java:
##########
@@ -102,4 +102,9 @@ public List<FunctionSignature> getSignatures() {
public Expression resultForEmptyInput() {
return new ArrayLiteral(new ArrayList<>(), this.getDataType());
}
+
+ @Override
+ public List<Expression> getDistinctArguments() {
+ return distinct ? ImmutableList.of(getArgument(0)) :
ImmutableList.of();
Review Comment:
[P1] Do not drop a slot-valued max_size from the distinct plan
The two-argument constructor still accepts `collect_list(DISTINCT a, w)`
with a column as `max_size`; there is no legality check requiring argument 1 to
be constant. Returning only `a` here makes every lower distinct phase output
`a` (or the group keys plus `a`), while the upper phase reconstructs
`collect_list(a, w)` from all original children. That leaves `w` unbound even
though the BE implementation reads `columns[1]`. Either reject non-constant
limits during analysis or retain argument 1 whenever it is not a proven
constant, and cover both scalar and grouped two-argument forms.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctAggStrategySelector.java:
##########
@@ -119,19 +119,20 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, Distinct
}
private boolean shouldUseMultiDistinct(LogicalAggregate<? extends Plan>
agg) {
- boolean mustUseCte =
AggregateUtils.containsCountDistinctMultiExpr(agg);
+ boolean mustUseCte =
AggregateUtils.containsNotSupportMultiDistinctFunction(agg);
boolean mustUseMulti = agg.getSourceRepeat().isPresent();
if (mustUseCte && mustUseMulti) {
Review Comment:
[P1] Exclude every mandatory control before rejecting GROUPING
This condition assumes `getDistinctArguments()` already describes
row-deduplication keys for every accepted aggregate, but some unsupported
DISTINCT functions still inherit the all-children default. For example,
`percentile_reservoir(DISTINCT a, 0.25)` plus the `0.75` form should share the
single row key `a`; likewise two `exponential_moving_average` calls with
different constant half-decays share `(value,time)`. They are currently counted
as two groups, so adding ROLLUP/GROUPING SETS reaches this new exception and
rejects a query that satisfies the stated single-group supported case. Define
the control-free key contract for all such functions before using the count
here, and test same keys with different controls under Repeat.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DistinctAggStrategySelector.java:
##########
@@ -119,19 +119,20 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, Distinct
}
private boolean shouldUseMultiDistinct(LogicalAggregate<? extends Plan>
agg) {
- boolean mustUseCte =
AggregateUtils.containsCountDistinctMultiExpr(agg);
+ boolean mustUseCte =
AggregateUtils.containsNotSupportMultiDistinctFunction(agg);
Review Comment:
[P1] Handle guarded aggregates before forcing the CTE split
A Doris view can normalize an output as
`Alias(SessionVarGuardExpr(Sum(DISTINCT a)))` when its persisted
result-affecting session variables differ from the querying session. The deep
scans used here still see that sum and, when it is paired with an unsupported
aggregate such as `array_agg(DISTINCT b)`, this line forces the CTE strategy.
However, `SplitMultiDistinctStrategy.collectDistinctAndNonDistinctFunctions`
only accepts an alias whose immediate child is an `AggregateFunction`, so it
silently drops the guarded sum, creates one branch, and `constructJoin`
dereferences `newAggs.get(1)`. Please unwrap/re-wrap transparent guards in the
splitter (preserving their session map) and add a view/session-mismatch case
before routing this broader surface through CTE.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeAggregate.java:
##########
@@ -195,7 +194,7 @@ private LogicalPlan normalizeAgg(LogicalAggregate<Plan>
aggregate, Optional<Logi
for (Expression arg : aggFunc.children()) {
// should not push down literal under aggregate
// e.g. group_concat(distinct xxx, ','), the ',' literal
show stay in aggregate
- if (arg instanceof Literal) {
+ if (arg.isConstant()) {
Review Comment:
[P2] Preserve row-wise evaluation for observable constants
`isConstant()` is broader than expressions that are safe to move across
DISTINCT deduplication. For example, `CAST(sleep(1) AS DOUBLE)` is classified
constant and is accepted as a `percentile(DISTINCT a, ...)` level, but BE
`FunctionSleep` deliberately expands constants and sleeps once per input row.
Previously normalization projected this expression below the aggregate, so it
ran on raw rows; this branch now leaves it in the later aggregate while the new
distinct-key override omits the level, so it runs only on deduplicated rows.
Please use a defer-safe predicate (or mark row-observable functions
accordingly) instead of bare `isConstant()`.
--
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]
