[fix](agg) Fix distinct aggregate argument grouping and strategy selection (#65368)

### What problem does this PR solve?

Problem Summary:

The Nereids distinct aggregate planning logic had several correctness
issues:

1. `CheckMultiDistinct` determined distinct groups from all aggregate
arguments instead of the semantic DISTINCT arguments. This incorrectly
treated separators and ORDER BY expressions as deduplication keys and
mishandled aggregates whose DISTINCT argument sets were equivalent but
ordered differently, such as `COUNT(DISTINCT a, b)` and `COUNT(DISTINCT b, a)`.

2. `GROUP_CONCAT` accepted a column expression as its separator. During
distinct aggregate rewriting, this extra input could incorrectly participate
in deduplication even though DISTINCT semantics only apply to the value argument.

3. Forced `agg_phase=3/4` could override `GROUP_CONCAT(DISTINCT ...
ORDER BY ...)`'s requirement to use the multi-distinct implementation.
Conversely, early logical splitting could prevent forced three/four-phase
aggregation from being honored.

4. `AggregateUtils#getDistinctNamedExpr` collected all aggregate
arguments rather than only semantic DISTINCT arguments.

This PR:

- Compares DISTINCT aggregates by their semantic DISTINCT argument groups.
- Treats equivalent argument sets as one distinct group.
- Requires the separator of `GROUP_CONCAT` and
`MULTI_DISTINCT_GROUP_CONCAT` to be a constant.
- Introduces explicit strategy selection between logical splitting,
multi-distinct aggregation, and Cascades multi-phase splitting.
- Preserves `mustUseMultiDistinctAgg()` when `agg_phase` is forced.
- Lets `agg_phase=3/4` retain the original DISTINCT aggregate for Cascades phase planning.
- Rejects queries that combine `GROUP_CONCAT(DISTINCT ... ORDER BY ...)` with a multi-argument `COUNT(DISTINCT ...)`, because their required aggregation strategies are incompatible.
- Uses `AggregateFunction#getDistinctArguments()` when collecting distinct deduplication keys.

### Release note

`GROUP_CONCAT` and `MULTI_DISTINCT_GROUP_CONCAT` now require the
separator argument to be a constant.

Queries combining `GROUP_CONCAT(DISTINCT ... ORDER BY ...)` with a
multi-argument `COUNT(DISTINCT ...)` are rejected with a clear analysis
error.
This commit is contained in:
feiniaofeiafei authored and morrySnow committed 2026-11-03 10:45:32 +08:00
1 parent dd72933ec7
commit ae63702744
15 files changed
+159 -78

No files matched your search

@@ -76,7 +76,7 @@ public class SplitAggMultiPhase extends SplitAggBaseRule implements Implementati
return ImmutableList.of(
logicalAggregate()
.when(agg -> !agg.getGroupByExpressions().isEmpty())
.when(agg -> agg.getDistinctArguments().size() == 1 || agg.distinctFuncNum() == 1)
.when(agg -> AggregateUtils.distinctArgumentGroupCountUpToTwo(agg) == 1)
.thenApplyMulti(this::rewrite)
.toRule(RuleType.SPLIT_AGG_MULTI_PHASE)
);
@@ -75,7 +75,7 @@ public class SplitAggMultiPhaseWithoutGbyKey extends SplitAggBaseRule implements
return ImmutableList.of(
logicalAggregate()
.when(agg -> agg.getGroupByExpressions().isEmpty())
.when(agg -> agg.getDistinctArguments().size() == 1 || agg.distinctFuncNum() == 1)
.when(agg -> AggregateUtils.distinctArgumentGroupCountUpToTwo(agg) == 1)
.thenApplyMulti(ctx -> rewrite(ctx.root, ctx.cascadesContext))
.toRule(RuleType.SPLIT_AGG_MULTI_PHASE_WITHOUT_GBY_KEY)
);
@@ -20,20 +20,12 @@ package org.apache.doris.nereids.rules.rewrite;
import org.apache.doris.nereids.exceptions.AnalysisException;
import org.apache.doris.nereids.rules.Rule;
import org.apache.doris.nereids.rules.RuleType;
import org.apache.doris.nereids.trees.expressions.OrderExpression;
import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction;
import org.apache.doris.nereids.trees.expressions.functions.agg.SupportMultiDistinct;
import org.apache.doris.nereids.trees.plans.Plan;
import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
import org.apache.doris.nereids.util.AggregateUtils;
/**
* If there are multiple distinct aggregate functions that cannot
* be transformed into multi_distinct, an error is reported.
* The following functions can be transformed into multi_distinct:
* - count -> MULTI_DISTINCT_COUNT
* - sum -> MULTI_DISTINCT_SUM
* - avg -> MULTI_DISTINCT_AVG
* - group_concat -> MULTI_DISTINCT_GROUP_CONCAT
* Check that previous aggregate rewrite rules have already handled multiple distinct argument groups.
*/
public class CheckMultiDistinct extends OneRewriteRuleFactory {
@Override
@@ -42,43 +34,9 @@ public class CheckMultiDistinct extends OneRewriteRuleFactory {
}
private LogicalAggregate checkDistinct(LogicalAggregate<? extends Plan> aggregate) {
if (aggregate.getDistinctArguments().size() > 1) {
for (AggregateFunction func : aggregate.getAggregateFunctions()) {
if (func.isDistinct() && !(func instanceof SupportMultiDistinct)) {
throw new AnalysisException(func.toString() + " can't support multi distinct.");
}
}
}
boolean distinctMultiColumns = false;
for (AggregateFunction func : aggregate.getAggregateFunctions()) {
if (!func.isDistinct()) {
continue;
}
if (func.arity() <= 1) {
continue;
}
for (int i = 1; i < func.arity(); i++) {
if (!func.child(i).getInputSlots().isEmpty() && !(func.child(i) instanceof OrderExpression)) {
// think about group_concat(distinct col_1, ',')
distinctMultiColumns = true;
break;
}
}
if (distinctMultiColumns) {
break;
}
}
long distinctFunctionNum = 0;
for (AggregateFunction aggregateFunction : aggregate.getAggregateFunctions()) {
distinctFunctionNum += aggregateFunction.isDistinct() ? 1 : 0;
}
if (distinctMultiColumns && distinctFunctionNum > 1) {
if (AggregateUtils.distinctArgumentGroupCountUpToTwo(aggregate) > 1) {
throw new AnalysisException(
"The query contains multi count distinct or sum distinct, each can't have multi columns");
"Multiple DISTINCT aggregate functions with different argument lists are not supported.");
}
return aggregate;
}
@@ -103,11 +103,12 @@ public class DistinctAggStrategySelector extends DefaultPlanRewriter<DistinctSel
// not process:
// count(distinct a,b);
// count(distinct a), sum(distinct a);
// count(distinct a,b), count(distinct b,a);
// count(distinct a)
// process:
// count(distinct a,b), count(distinct a,c)
// count(distinct a), sum(distinct b)
if (agg.distinctFuncNum() < 2 || agg.getDistinctArguments().size() < 2) {
if (AggregateUtils.distinctArgumentGroupCountUpToTwo(agg) <= 1) {
return agg;
}
if (shouldUseMultiDistinct(agg)) {
@@ -21,6 +21,7 @@ import org.apache.doris.catalog.Column;
import org.apache.doris.catalog.DistributionInfo;
import org.apache.doris.catalog.HashDistributionInfo;
import org.apache.doris.catalog.OlapTable;
import org.apache.doris.nereids.exceptions.AnalysisException;
import org.apache.doris.nereids.rules.Rule;
import org.apache.doris.nereids.rules.RuleType;
import org.apache.doris.nereids.rules.rewrite.StatsDerive.DeriveContext;
@@ -83,6 +84,13 @@ public class DistinctAggregateRewriter implements RewriteRuleFactory {
private static final Set<Class<? extends AggregateFunction>> supportSplitOtherFunctions = ImmutableSet.of(
Sum.class, Min.class, Max.class, Count.class, Sum0.class, AnyValue.class);
enum Strategy {
SPLIT_IN_REWRITE, MULTI_STRATEGY, SPLIT_IN_CASCADES
}
private static final String errorString = "Unsupported query: GROUP_CONCAT(DISTINCT ... ORDER BY ...)"
+ " cannot be used together with a multi-argument COUNT(DISTINCT ...).";
@Override
public List<Rule> buildRules() {
return ImmutableList.of(
@@ -92,25 +100,41 @@ public class DistinctAggregateRewriter implements RewriteRuleFactory {
.thenApply(ctx -> rewrite(ctx.root, ctx.connectContext))
.toRule(RuleType.DISTINCT_AGGREGATE_SPLIT),
logicalAggregate()
.when(agg -> agg.getGroupByExpressions().isEmpty()
&& agg.mustUseMultiDistinctAgg() && !AggregateUtils.containsCountDistinctMultiExpr(agg))
.then(this::convertToMultiDistinct)
.when(agg -> agg.getGroupByExpressions().isEmpty() && agg.mustUseMultiDistinctAgg())
.then(agg -> {
// count(distinct a,b) cannot use multi_distinct
if (AggregateUtils.containsCountDistinctMultiExpr(agg)) {
throw new AnalysisException(errorString);
}
return convertToMultiDistinct(agg);
})
.toRule(RuleType.PROCESS_SCALAR_AGG_MUST_USE_MULTI_DISTINCT)
);
}
@VisibleForTesting
boolean shouldUseMultiDistinct(LogicalAggregate<? extends Plan> aggregate) {
Strategy chooseStrategy(LogicalAggregate<? extends Plan> aggregate) {
// count(distinct a,b) cannot use multi_distinct
if (AggregateUtils.containsCountDistinctMultiExpr(aggregate)) {
return false;
}
if (aggregate.mustUseMultiDistinctAgg()) {
return true;
boolean mustSplit = AggregateUtils.containsCountDistinctMultiExpr(aggregate);
boolean mustUseMulti = aggregate.mustUseMultiDistinctAgg();
if (mustSplit && mustUseMulti) {
throw new AnalysisException(errorString);
}
ConnectContext ctx = ConnectContext.get();
if (mustSplit) {
if (ctx.getSessionVariable().aggPhase == 3 || ctx.getSessionVariable().aggPhase == 4) {
return Strategy.SPLIT_IN_CASCADES;
}
return Strategy.SPLIT_IN_REWRITE;
}
if (mustUseMulti) {
return Strategy.MULTI_STRATEGY;
}
if (ctx.getSessionVariable().aggPhase == 1 || ctx.getSessionVariable().aggPhase == 2) {
return true;
return Strategy.MULTI_STRATEGY;
}
if (ctx.getSessionVariable().aggPhase == 3 || ctx.getSessionVariable().aggPhase == 4) {
return Strategy.SPLIT_IN_CASCADES;
}
if (aggregate.getStats() == null || aggregate.child().getStats() == null) {
StatsDerive derive = new StatsDerive(false);
@@ -120,12 +144,12 @@ public class DistinctAggregateRewriter implements RewriteRuleFactory {
Statistics aggChildStats = aggregate.child().getStats();
Set<Expression> dstArgs = aggregate.getDistinctArguments();
if (isDistinctKeySatisfyDistribution(aggregate)) {
return false;
return Strategy.SPLIT_IN_REWRITE;
}
// has unknown statistics, split to bottom and top agg
if (AggregateUtils.hasUnknownStatistics(aggregate.getGroupByExpressions(), aggChildStats)
|| AggregateUtils.hasUnknownStatistics(dstArgs, aggChildStats)) {
return true;
return Strategy.MULTI_STRATEGY;
}
double gbyNdv = aggStats.getRowCount();
@@ -138,8 +162,12 @@ public class DistinctAggregateRewriter implements RewriteRuleFactory {
double inputRows = aggChildStats.getRowCount();
// group by key ndv is low, distinct key ndv is high, multi_distinct is better
// otherwise split to bottom and top agg
return gbyNdv < inputRows * AggregateUtils.LOW_CARDINALITY_THRESHOLD
&& dstNdv > inputRows * AggregateUtils.HIGH_CARDINALITY_THRESHOLD;
if (gbyNdv < inputRows * AggregateUtils.LOW_CARDINALITY_THRESHOLD
&& dstNdv > inputRows * AggregateUtils.HIGH_CARDINALITY_THRESHOLD) {
return Strategy.MULTI_STRATEGY;
} else {
return Strategy.SPLIT_IN_REWRITE;
}
}
private boolean isDistinctKeySatisfyDistribution(LogicalAggregate<? extends Plan> aggregate) {
@@ -249,14 +277,15 @@ public class DistinctAggregateRewriter implements RewriteRuleFactory {
if (aggregate.distinctFuncNum() == 0) {
return null;
}
if (ctx.getSessionVariable().aggPhase == 3 || ctx.getSessionVariable().aggPhase == 4) {
Strategy strategy = chooseStrategy(aggregate);
if (strategy == Strategy.MULTI_STRATEGY) {
return convertToMultiDistinct(aggregate);
} else if (strategy == Strategy.SPLIT_IN_REWRITE) {
return splitDistinctAgg(aggregate);
} else if (strategy == Strategy.SPLIT_IN_CASCADES) {
return null;
}
if (shouldUseMultiDistinct(aggregate)) {
return convertToMultiDistinct(aggregate);
} else {
return splitDistinctAgg(aggregate);
}
return null;
}
private Plan convertToMultiDistinct(LogicalAggregate<? extends Plan> aggregate) {
@@ -201,4 +201,12 @@ public class GroupConcat extends NullableAggregateFunction
}
return firstOrderExrIndex;
}
@Override
public void checkLegalityBeforeTypeCoercion() {
if (nonOrderArguments == 2 && !getArgument(1).isConstant()) {
throw new AnalysisException(
"group_concat requires separator must be a constant : " + this.toSql());
}
}
}
@@ -140,4 +140,12 @@ public class MultiDistinctGroupConcat extends NullableAggregateFunction
}
return firstOrderExrIndex;
}
@Override
public void checkLegalityBeforeTypeCoercion() {
if (nonOrderArguments == 2 && !getArgument(1).isConstant()) {
throw new AnalysisException(
"multi_distinct_group_concat requires separator must be a constant : " + this.toSql());
}
}
}
@@ -29,6 +29,7 @@ import org.apache.doris.nereids.trees.expressions.functions.agg.SupportMultiDist
import org.apache.doris.nereids.trees.expressions.functions.scalar.If;
import org.apache.doris.nereids.trees.expressions.literal.NullLiteral;
import org.apache.doris.nereids.trees.plans.Plan;
import org.apache.doris.nereids.trees.plans.algebra.Aggregate;
import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
import org.apache.doris.qe.ConnectContext;
import org.apache.doris.statistics.ColumnStatistic;
@@ -134,6 +135,23 @@ public class AggregateUtils {
expr instanceof Count && ((Count) expr).isDistinct() && expr.arity() > 1);
}
/** count agg function distinct group, up to 2*/
public static int distinctArgumentGroupCountUpToTwo(Aggregate<? extends Plan> aggregate) {
Set<Expression> distinctArgumentGroup = null;
for (AggregateFunction aggregateFunction : aggregate.getAggregateFunctions()) {
if (!aggregateFunction.isDistinct()) {
continue;
}
Set<Expression> currentGroup = ImmutableSet.copyOf(aggregateFunction.getDistinctArguments());
if (distinctArgumentGroup == null) {
distinctArgumentGroup = currentGroup;
} else if (!distinctArgumentGroup.equals(currentGroup)) {
return 2;
}
}
return distinctArgumentGroup == null ? 0 : 1;
}
/**getAllKeySet*/
public static Set<NamedExpression> getAllKeySet(LogicalAggregate<? extends Plan> aggregate) {
Set<NamedExpression> distinctArguments = getDistinctNamedExpr(aggregate);
@@ -156,7 +174,7 @@ public class AggregateUtils {
public static Set<NamedExpression> getDistinctNamedExpr(LogicalAggregate<? extends Plan> aggregate) {
return aggregate.getAggregateFunctions().stream()
.filter(AggregateFunction::isDistinct)
.flatMap(aggFunc -> aggFunc.getArguments().stream())
.flatMap(aggFunc -> aggFunc.getDistinctArguments().stream())
.filter(NamedExpression.class::isInstance)
.map(NamedExpression.class::cast)
.collect(ImmutableSet.toImmutableSet());
@@ -20,6 +20,7 @@ package org.apache.doris.nereids.rules.rewrite;
import org.apache.doris.catalog.DistributionInfo;
import org.apache.doris.catalog.HashDistributionInfo;
import org.apache.doris.nereids.rules.analysis.LogicalSubQueryAliasToLogicalProject;
import org.apache.doris.nereids.rules.rewrite.DistinctAggregateRewriter.Strategy;
import org.apache.doris.nereids.trees.expressions.SlotReference;
import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction;
import org.apache.doris.nereids.trees.expressions.functions.agg.Count;
@@ -249,7 +250,7 @@ public class DistinctAggregateRewriterTest extends TestWithFeService implements
}
@Test
void testShouldUseMultiDistinctWithoutStatsSatisfyDistribution() throws Exception {
void testChooseStrategyWithoutStatsSatisfyDistribution() throws Exception {
DistinctAggregateRewriter rewriter = DistinctAggregateRewriter.INSTANCE;
LogicalAggregate<? extends Plan> aggregate = getLogicalAggregate(
"select bb, count(distinct aa) from "
@@ -265,11 +266,11 @@ public class DistinctAggregateRewriterTest extends TestWithFeService implements
((AbstractPlan) child).setStatistics(new Statistics(10000, colStats));
aggregate.setStatistics(new Statistics(100, ImmutableMap.of()));
Assertions.assertFalse(rewriter.shouldUseMultiDistinct(aggregate));
Assertions.assertEquals(Strategy.SPLIT_IN_REWRITE, rewriter.chooseStrategy(aggregate));
}
@Test
void testShouldUseMultiDistinctWithStatsSelected() throws Exception {
void testChooseStrategyWithStatsSelected() throws Exception {
DistinctAggregateRewriter rewriter = new DistinctAggregateRewriter();
LogicalAggregate<? extends Plan> aggregate = getLogicalAggregate(
"select b, count(distinct a) from test.distinct_agg_split_t group by b"
@@ -283,11 +284,11 @@ public class DistinctAggregateRewriterTest extends TestWithFeService implements
((AbstractPlan) child).setStatistics(new Statistics(100000, colStats));
aggregate.setStatistics(new Statistics(240, ImmutableMap.of()));
Assertions.assertFalse(rewriter.shouldUseMultiDistinct(aggregate));
Assertions.assertEquals(Strategy.SPLIT_IN_REWRITE, rewriter.chooseStrategy(aggregate));
}
@Test
void testShouldUseMultiDistinctWithPartitionTable() {
void testChooseStrategyWithPartitionTable() {
DistinctAggregateRewriter rewriter = DistinctAggregateRewriter.INSTANCE;
LogicalAggregate<? extends Plan> aggregate = getLogicalAggregate(
"select count(distinct record_id) from sales_records group by sale_date;"
@@ -301,7 +302,7 @@ public class DistinctAggregateRewriterTest extends TestWithFeService implements
((AbstractPlan) child).setStatistics(new Statistics(10000, colStats));
aggregate.setStatistics(new Statistics(100, ImmutableMap.of()));
Assertions.assertTrue(rewriter.shouldUseMultiDistinct(aggregate));
Assertions.assertEquals(Strategy.MULTI_STRATEGY, rewriter.chooseStrategy(aggregate));
}
@Test
@@ -38,3 +38,12 @@ PhysicalResultSink
--------hashAgg[LOCAL]
----------PhysicalOlapScan[t1000_2]
-- !agg_phase3 --
PhysicalResultSink
--PhysicalDistribute[DistributionSpecGather]
----hashAgg[DISTINCT_GLOBAL]
------hashAgg[GLOBAL]
--------PhysicalDistribute[DistributionSpecHash]
----------hashAgg[LOCAL]
------------PhysicalTVFRelation
@@ -110,3 +110,18 @@ PhysicalResultSink
--------hashAgg[LOCAL]
----------PhysicalOlapScan[t1000]
-- !count_distinct_group --
5 5
-- !count_distinct_group_with_gby --
1 1
1 1
1 1
1 1
1 1
1 1
1 1
1 1
1 1
1 1
@@ -133,4 +133,13 @@ suite("agg_group_concat") {
insert into test_distinct_multi values(1,2,3,'abc','2024-01-02'),(1,2,4,'abc','2024-01-03'),(2,2,4,'abcd','2024-01-02'),(1,2,3,'abcd','2024-01-04'),(1,2,4,'eee','2024-02-02'),(2,2,4,'abc','2024-01-02');
"""
qt_test "select group_concat( distinct d order by d) from test_distinct_multi order by 1; "
test {
sql "select multi_distinct_GROUP_CONCAT(a, c) from test_distinct_multi group by b; "
exception "separator must be a constant"
}
test {
sql "select GROUP_CONCAT(a, c order by c) from test_distinct_multi group by b; "
exception "separator must be a constant"
}
}
@@ -33,4 +33,25 @@ suite("distinct_agg_rewriter") {
select count(distinct d_200) from t1000_2 group by b_5;"""
qt_use_multi_distinct """explain shape plan
select count(distinct d_200) from t1000_2 group by a_1;"""
}
sql "set agg_phase=4"
sql "select group_concat(distinct dst_key1 order by dst_key2) from t_gbykey_10_dstkey_10_1000_id group by gby_key"
test {
sql """select number % 2, count(distinct cast(number as varchar), cast(number as varchar)),
group_concat(distinct cast(number as varchar) order by number + 1) from numbers('number'='10') group by number % 2"""
exception "Unsupported query"
}
test {
sql """select count(distinct cast(number as varchar), cast(number as varchar)),
group_concat(distinct cast(number as varchar) order by number + 1) from numbers('number'='10')"""
exception "Unsupported query"
}
// expect to have 3 HashAgg instead of 4 HashAgg
sql "set agg_phase=3"
qt_agg_phase3 """explain shape plan
select number % 2, count(distinct number)
from numbers('number'='10')
group by number % 2;"""
}
@@ -52,4 +52,8 @@ suite("distinct_agg_strategy_selector") {
contains "multi_distinct_count"
}
sql "set multi_distinct_strategy=0 "
// test
order_qt_count_distinct_group "select count(distinct a_1,b_5), count(distinct b_5,a_1) from t1000;"
order_qt_count_distinct_group_with_gby "select count(distinct a_1,b_5), count(distinct b_5,a_1) from t1000 group by c_10;"
}
@@ -91,7 +91,7 @@ suite("test_group_concat", "query,p0,arrow_flight_sql") {
// test SPLIT_MULTI_DISTINCT could work right with can not be banned aggregation
qt_select_13 """
select
group_concat( distinct b1, cast(b2 as varchar)), group_concat( distinct b3, '?')
group_concat( distinct b1, ','), group_concat( distinct b3, '?')
from
table_group_concat
group by