You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by mo...@apache.org on 2024/06/13 02:32:39 UTC
(doris) branch master updated: [Fix](nereids) fix merge aggregate rule, rules should not have mutable members (#36145)
This is an automated email from the ASF dual-hosted git repository.
morrysnow pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new 5c0adca2c84 [Fix](nereids) fix merge aggregate rule, rules should not have mutable members (#36145)
5c0adca2c84 is described below
commit 5c0adca2c846f86a0c5b34d328c67d3c58c276e4
Author: feiniaofeiafei <53...@users.noreply.github.com>
AuthorDate: Thu Jun 13 10:32:31 2024 +0800
[Fix](nereids) fix merge aggregate rule, rules should not have mutable members (#36145)
This bug is induced by #31811.
The innerAggExprIdToAggFunc was a member of MergeAggregate, which was
wrong. Because rules like MergeAggregate are single instances, same
rules applied to different sub-plans will affect each other. This pr
changes innerAggExprIdToAggFunc to a local variable, fixes this bug.
No regression use case was added because it’s not a problem that will
definitely reoccur and requires the same rule to be applied to multiple
plans at the same time.
---
.../doris/nereids/rules/rewrite/MergeAggregate.java | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/MergeAggregate.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/MergeAggregate.java
index 23fbd978656..4b9e745ee20 100644
--- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/MergeAggregate.java
+++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/MergeAggregate.java
@@ -52,7 +52,6 @@ import java.util.stream.Collectors;
public class MergeAggregate implements RewriteRuleFactory {
private static final ImmutableSet<String> ALLOW_MERGE_AGGREGATE_FUNCTIONS =
ImmutableSet.of("min", "max", "sum", "any_value");
- private Map<ExprId, AggregateFunction> innerAggExprIdToAggFunc = new HashMap<>();
@Override
public List<Rule> buildRules() {
@@ -75,7 +74,7 @@ public class MergeAggregate implements RewriteRuleFactory {
*/
private Plan mergeTwoAggregate(LogicalAggregate<LogicalAggregate<Plan>> outerAgg) {
LogicalAggregate<Plan> innerAgg = outerAgg.child();
-
+ Map<ExprId, AggregateFunction> innerAggExprIdToAggFunc = getInnerAggExprIdToAggFuncMap(innerAgg);
List<NamedExpression> newOutputExpressions = outerAgg.getOutputExpressions().stream()
.map(e -> rewriteAggregateFunction(e, innerAggExprIdToAggFunc))
.collect(Collectors.toList());
@@ -97,6 +96,7 @@ public class MergeAggregate implements RewriteRuleFactory {
List<NamedExpression> outputExpressions = outerAgg.getOutputExpressions();
List<NamedExpression> replacedOutputExpressions = PlanUtils.replaceExpressionByProjections(
project.getProjects(), (List) outputExpressions);
+ Map<ExprId, AggregateFunction> innerAggExprIdToAggFunc = getInnerAggExprIdToAggFuncMap(innerAgg);
// rewrite agg function. e.g. max(max)
List<NamedExpression> replacedAggFunc = replacedOutputExpressions.stream()
.filter(expr -> (expr instanceof Alias) && (expr.child(0) instanceof AggregateFunction))
@@ -152,10 +152,7 @@ public class MergeAggregate implements RewriteRuleFactory {
private boolean commonCheck(LogicalAggregate<? extends Plan> outerAgg, LogicalAggregate<Plan> innerAgg,
boolean sameGroupBy, Optional<LogicalProject> projectOptional) {
- innerAggExprIdToAggFunc = innerAgg.getOutputExpressions().stream()
- .filter(expr -> (expr instanceof Alias) && (expr.child(0) instanceof AggregateFunction))
- .collect(Collectors.toMap(NamedExpression::getExprId, value -> (AggregateFunction) value.child(0),
- (existValue, newValue) -> existValue));
+ Map<ExprId, AggregateFunction> innerAggExprIdToAggFunc = getInnerAggExprIdToAggFuncMap(innerAgg);
Set<AggregateFunction> aggregateFunctions = outerAgg.getAggregateFunctions();
List<AggregateFunction> replacedAggFunctions = projectOptional.map(project ->
(List<AggregateFunction>) PlanUtils.replaceExpressionByProjections(
@@ -225,4 +222,11 @@ public class MergeAggregate implements RewriteRuleFactory {
boolean sameGroupBy = (innerAgg.getGroupByExpressions().size() == outerAgg.getGroupByExpressions().size());
return commonCheck(outerAgg, innerAgg, sameGroupBy, Optional.of(project));
}
+
+ private Map<ExprId, AggregateFunction> getInnerAggExprIdToAggFuncMap(LogicalAggregate<Plan> innerAgg) {
+ return innerAgg.getOutputExpressions().stream()
+ .filter(expr -> (expr instanceof Alias) && (expr.child(0) instanceof AggregateFunction))
+ .collect(Collectors.toMap(NamedExpression::getExprId, value -> (AggregateFunction) value.child(0),
+ (existValue, newValue) -> existValue));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org