morrySnow commented on code in PR #10659:
URL: https://github.com/apache/doris/pull/10659#discussion_r915563444


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggregateDisassemble.java:
##########
@@ -64,49 +65,69 @@ public Rule<Plan> build() {
             Operator operator = plan.getOperator();
             LogicalAggregate agg = (LogicalAggregate) operator;
             List<NamedExpression> outputExpressionList = 
agg.getOutputExpressionList();
-            List<NamedExpression> intermediateAggExpressionList = 
Lists.newArrayList();
-            // TODO: shouldn't extract agg function from this field.
-            for (NamedExpression namedExpression : outputExpressionList) {
-                namedExpression = (NamedExpression) namedExpression.clone();
-                List<AggregateFunction> functionCallList =
-                        
namedExpression.collect(org.apache.doris.catalog.AggregateFunction.class::isInstance);
-                // TODO: we will have another mechanism to get corresponding 
stale agg func.
-                for (AggregateFunction functionCall : functionCallList) {
-                    org.apache.doris.catalog.AggregateFunction staleAggFunc = 
findAggFunc(functionCall);
-                    Type staleIntermediateType = 
staleAggFunc.getIntermediateType();
-                    Type staleRetType = staleAggFunc.getReturnType();
-                    if (staleIntermediateType != null && 
!staleIntermediateType.equals(staleRetType)) {
-                        
functionCall.setIntermediate(DataType.convertFromCatalogDataType(staleIntermediateType));
+            List<Expression> groupByExpressionList = 
agg.getGroupByExpressionList();
+
+            Map<AggregateFunction, NamedExpression> aggregateFunctionAliasMap 
= Maps.newHashMap();
+            for (NamedExpression outputExpression : outputExpressionList) {
+                outputExpression.foreach(e -> {
+                    if (e instanceof AggregateFunction) {
+                        AggregateFunction a = (AggregateFunction) e;
+                        aggregateFunctionAliasMap.put(a, new Alias<>(a, 
a.sql()));
+                    }
+                });
+            }
+
+            List<Expression> updateGroupByExpressionList = 
groupByExpressionList;
+            List<NamedExpression> updateGroupByAliasList = 
updateGroupByExpressionList.stream()
+                    .map(g -> new Alias<>(g, g.sql()))
+                    .collect(Collectors.toList());
+
+            List<NamedExpression> updateOutputExpressionList = 
Lists.newArrayList();
+            updateOutputExpressionList.addAll(updateGroupByAliasList);
+            
updateOutputExpressionList.addAll(aggregateFunctionAliasMap.values());
+
+            List<Expression> mergeGroupByExpressionList = 
updateGroupByAliasList.stream()
+                    .map(NamedExpression::toSlot).collect(Collectors.toList());
+
+            List<NamedExpression> mergeOutputExpressionList = 
Lists.newArrayList();
+            for (NamedExpression o : outputExpressionList) {
+                if (o.contains(AggregateFunction.class::isInstance)) {
+                    mergeOutputExpressionList.add((NamedExpression) new 
AggregateFunctionParamsRewriter()
+                            .visit(o, aggregateFunctionAliasMap));
+                } else {
+                    for (int i = 0; i < updateGroupByAliasList.size(); i++) {
+                        // TODO: we need to do sub tree match and replace. but 
we do not have semanticEquals now.
+                        //    e.g. a + 1 + 2 in output expression should be 
replaced by
+                        //    (slot reference to update phase out (a + 1)) + 
2, if we do group by a + 1
+                        //   currently, we could only handle output expression 
same with group by expression
+                        if (o instanceof SlotReference) {
+                            // a in output expression will be SLotReference
+                            if (o.equals(updateGroupByExpressionList.get(i))) {
+                                
mergeOutputExpressionList.add(updateGroupByAliasList.get(i).toSlot());
+                                break;
+                            }
+                        } else if (o instanceof Alias) {
+                            // a + 1 in output expression will be Alias
+                            if 
(o.child(0).equals(updateGroupByExpressionList.get(i))) {
+                                
mergeOutputExpressionList.add(updateGroupByAliasList.get(i).toSlot());
+                                break;
+                            }
+                        }
                     }
                 }
-                intermediateAggExpressionList.add(namedExpression);
             }
+
             LogicalAggregate localAgg = new LogicalAggregate(
-                    
agg.getGroupByExprList().stream().map(Expression::clone).collect(Collectors.toList()),
-                    intermediateAggExpressionList,
+                    updateGroupByExpressionList,
+                    updateOutputExpressionList,

Review Comment:
   i use the names in the stale planner. imo, local and global is better than 
update and merge. but as discussion before, we want to reuse names in stale 
planner as much as possible. so i reserve these names.



-- 
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: commits-unsubscr...@doris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org

Reply via email to