Skip to content

Commit

Permalink
[fix](nereids) window function with grouping sets work not well (apac…
Browse files Browse the repository at this point in the history
…he#31475)

```sql        
select a, c, sum(sum(b)) over(partition by c order by c rows between unbounded preceding and current row) 
from test_window_table2 group by grouping sets((a),( c)) having a > 1 order by 1,2,3;
```
for this kind of case:
sum(sum(col)) over, nereids has cannot find slot problem.
the output slot of repeat and aggregate is computed wrongly.
Only collecting the trival-agg  in NormalizeRepeat can fix this problem.


Co-authored-by: feiniaofeiafei <[email protected]>
  • Loading branch information
feiniaofeiafei and feiniaofeiafei authored Feb 29, 2024
1 parent bd4bb58 commit d615624
Show file tree
Hide file tree
Showing 5 changed files with 76 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@
import org.apache.doris.nereids.trees.expressions.WindowExpression;
import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction;
import org.apache.doris.nereids.trees.expressions.literal.Literal;
import org.apache.doris.nereids.trees.expressions.visitor.DefaultExpressionVisitor;
import org.apache.doris.nereids.trees.plans.Plan;
import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
import org.apache.doris.nereids.trees.plans.logical.LogicalHaving;
import org.apache.doris.nereids.trees.plans.logical.LogicalPlan;
import org.apache.doris.nereids.trees.plans.logical.LogicalProject;
import org.apache.doris.nereids.util.ExpressionUtils;
import org.apache.doris.nereids.util.PlanUtils;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableList.Builder;
Expand Down Expand Up @@ -145,7 +145,7 @@ private LogicalPlan normalizeAgg(LogicalAggregate<Plan> aggregate, Optional<Logi
// collect all trival-agg
List<NamedExpression> aggregateOutput = aggregate.getOutputExpressions();
List<AggregateFunction> aggFuncs = Lists.newArrayList();
aggregateOutput.forEach(o -> o.accept(CollectNonWindowedAggFuncs.INSTANCE, aggFuncs));
aggregateOutput.forEach(o -> o.accept(PlanUtils.CollectNonWindowedAggFuncs.INSTANCE, aggFuncs));

// split non-distinct agg child as two part
// TRUE part 1: need push down itself, if it contains subqury or window expression
Expand Down Expand Up @@ -291,23 +291,4 @@ private List<NamedExpression> normalizeOutput(List<NamedExpression> aggregateOut
}
return builder.build();
}

private static class CollectNonWindowedAggFuncs extends DefaultExpressionVisitor<Void, List<AggregateFunction>> {

private static final CollectNonWindowedAggFuncs INSTANCE = new CollectNonWindowedAggFuncs();

@Override
public Void visitWindow(WindowExpression windowExpression, List<AggregateFunction> context) {
for (Expression child : windowExpression.getExpressionsInWindowSpec()) {
child.accept(this, context);
}
return null;
}

@Override
public Void visitAggregateFunction(AggregateFunction aggregateFunction, List<AggregateFunction> context) {
context.add(aggregateFunction);
return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@
import org.apache.doris.nereids.trees.plans.logical.LogicalProject;
import org.apache.doris.nereids.trees.plans.logical.LogicalRepeat;
import org.apache.doris.nereids.util.ExpressionUtils;
import org.apache.doris.nereids.util.PlanUtils;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.common.collect.Sets.SetView;
Expand Down Expand Up @@ -169,8 +171,9 @@ private Set<Expression> collectNeedToSlotExpressions(LogicalRepeat<Plan> repeat)
.flatMap(function -> function.getArguments().stream())
.collect(ImmutableSet.toImmutableSet());

Set<AggregateFunction> aggregateFunctions = ExpressionUtils.collect(
repeat.getOutputExpressions(), AggregateFunction.class::isInstance);
List<AggregateFunction> aggregateFunctions = Lists.newArrayList();
repeat.getOutputExpressions().forEach(
o -> o.accept(PlanUtils.CollectNonWindowedAggFuncs.INSTANCE, aggregateFunctions));

ImmutableSet<Expression> argumentsOfAggregateFunction = aggregateFunctions.stream()
.flatMap(function -> function.getArguments().stream().map(arg -> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@
import org.apache.doris.nereids.trees.expressions.Expression;
import org.apache.doris.nereids.trees.expressions.NamedExpression;
import org.apache.doris.nereids.trees.expressions.Slot;
import org.apache.doris.nereids.trees.expressions.WindowExpression;
import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateFunction;
import org.apache.doris.nereids.trees.expressions.visitor.DefaultExpressionVisitor;
import org.apache.doris.nereids.trees.plans.Plan;
import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
import org.apache.doris.nereids.trees.plans.logical.LogicalCatalogRelation;
Expand Down Expand Up @@ -121,4 +124,26 @@ public static ImmutableSet<TableIf> getTableSet(LogicalPlan plan) {
.collect(ImmutableSet.toImmutableSet());
return resultSet;
}

/**
* collect non_window_agg_func
*/
public static class CollectNonWindowedAggFuncs extends DefaultExpressionVisitor<Void, List<AggregateFunction>> {

public static final CollectNonWindowedAggFuncs INSTANCE = new CollectNonWindowedAggFuncs();

@Override
public Void visitWindow(WindowExpression windowExpression, List<AggregateFunction> context) {
for (Expression child : windowExpression.getExpressionsInWindowSpec()) {
child.accept(this, context);
}
return null;
}

@Override
public Void visitAggregateFunction(AggregateFunction aggregateFunction, List<AggregateFunction> context) {
context.add(aggregateFunction);
return null;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- This file is automatically generated. You should know what you did if you want to edit this
-- !select1 --
2 \N 46.0000000000

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// 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.
suite("window_agg_grouping_sets") {
sql "SET enable_nereids_planner=true"
sql "SET enable_fallback_to_original_planner=false"
sql """
DROP TABLE IF EXISTS test_window_table2
"""

sql """
create table test_window_table2 ( a varchar(100) null, b decimalv3(18,10) null, c int, ) ENGINE=OLAP
DUPLICATE KEY(`a`) DISTRIBUTED BY HASH(`a`) BUCKETS 1 PROPERTIES
( "replication_allocation" = "tag.location.default: 1" );
"""

sql """
insert into test_window_table2 values("1", 1, 1),("1", 1, 2),("1", 2, 1),("1", 2, 2),
("2", 11, 1),("2", 11, 2),("2", 12, 1),("2", 12, 2);
"""

qt_select1 """
select a, c, sum(sum(b)) over(partition by c order by c rows between unbounded preceding and current row)
from test_window_table2 group by grouping sets((a),( c)) having a > 1 order by 1,2,3;
"""

}

0 comments on commit d615624

Please sign in to comment.