seawinde commented on code in PR #34274:
URL: https://github.com/apache/doris/pull/34274#discussion_r1591728872


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -230,57 +230,56 @@ protected Plan rewriteQueryByView(MatchMode matchMode,
         // split the query top plan expressions to group expressions and 
functions, if can not, bail out.
         Pair<Set<? extends Expression>, Set<? extends Expression>> 
queryGroupAndFunctionPair
                 = topPlanSplitToGroupAndFunction(queryTopPlanAndAggPair, 
queryStructInfo);
+        Set<? extends Expression> queryTopPlanGroupBySet = 
queryGroupAndFunctionPair.key();
         Set<? extends Expression> queryTopPlanFunctionSet = 
queryGroupAndFunctionPair.value();
         // try to rewrite, contains both roll up aggregate functions and 
aggregate group expression
         List<NamedExpression> finalOutputExpressions = new ArrayList<>();
         List<Expression> finalGroupExpressions = new ArrayList<>();
-        List<? extends Expression> queryExpressions = queryTopPlan.getOutput();
         // permute the mv expr mapping to query based
         Map<Expression, Expression> mvExprToMvScanExprQueryBased =
                 
materializationContext.getMvExprToMvScanExprMapping().keyPermute(viewToQuerySlotMapping)
                         .flattenMap().get(0);
-        for (Expression topExpression : queryExpressions) {
+        for (Expression topExpression : queryTopPlanGroupBySet) {
+            // if group by expression, try to rewrite group by expression
+            Expression queryGroupShuttledExpr = 
ExpressionUtils.shuttleExpressionWithLineage(
+                    topExpression, queryTopPlan, 
queryStructInfo.getTableBitSet());
+            AggregateExpressionRewriteContext context = new 
AggregateExpressionRewriteContext(true,
+                    mvExprToMvScanExprQueryBased, queryTopPlan, 
queryStructInfo.getTableBitSet());
+            // group by expression maybe group by a + b, so we need expression 
rewriter
+            Expression rewrittenGroupByExpression = 
queryGroupShuttledExpr.accept(AGGREGATE_EXPRESSION_REWRITER,
+                    context);
+            if (!context.isValid()) {
+                // group expr can not rewrite by view
+                materializationContext.recordFailReason(queryStructInfo,
+                        "View dimensions doesn't not cover the query 
dimensions",
+                        () -> String.format("mvExprToMvScanExprQueryBased is 
%s,\n queryGroupShuttledExpr is %s",
+                                mvExprToMvScanExprQueryBased, 
queryGroupShuttledExpr));
+                return null;
+            }
+            NamedExpression groupByExpression = rewrittenGroupByExpression 
instanceof NamedExpression
+                    ? (NamedExpression) rewrittenGroupByExpression : new 
Alias(rewrittenGroupByExpression);
+            finalOutputExpressions.add(groupByExpression);
+            finalGroupExpressions.add(groupByExpression);
+        }
+        for (Expression topExpression : queryTopPlanFunctionSet) {
             // if agg function, try to roll up and rewrite
-            if (queryTopPlanFunctionSet.contains(topExpression)) {
-                Expression queryFunctionShuttled = 
ExpressionUtils.shuttleExpressionWithLineage(
-                        topExpression,
-                        queryTopPlan,
-                        queryStructInfo.getTableBitSet());
-                AggregateExpressionRewriteContext context = new 
AggregateExpressionRewriteContext(
-                        false, mvExprToMvScanExprQueryBased, queryTopPlan, 
queryStructInfo.getTableBitSet());
-                // queryFunctionShuttled maybe sum(column) + count(*), so need 
to use expression rewriter
-                Expression rollupedExpression = 
queryFunctionShuttled.accept(AGGREGATE_EXPRESSION_REWRITER,
-                        context);
-                if (!context.isValid()) {
-                    materializationContext.recordFailReason(queryStructInfo,
-                            "Query function roll up fail",
-                            () -> String.format("queryFunctionShuttled = %s,\n 
mvExprToMvScanExprQueryBased = %s",
-                                    queryFunctionShuttled, 
mvExprToMvScanExprQueryBased));
-                    return null;
-                }
-                finalOutputExpressions.add(new Alias(rollupedExpression));
-            } else {
-                // if group by expression, try to rewrite group by expression
-                Expression queryGroupShuttledExpr = 
ExpressionUtils.shuttleExpressionWithLineage(
-                        topExpression, queryTopPlan, 
queryStructInfo.getTableBitSet());
-                AggregateExpressionRewriteContext context = new 
AggregateExpressionRewriteContext(true,
-                        mvExprToMvScanExprQueryBased, queryTopPlan, 
queryStructInfo.getTableBitSet());
-                // group by expression maybe group by a + b, so we need 
expression rewriter
-                Expression rewrittenGroupByExpression = 
queryGroupShuttledExpr.accept(AGGREGATE_EXPRESSION_REWRITER,
-                        context);
-                if (!context.isValid()) {
-                    // group expr can not rewrite by view
-                    materializationContext.recordFailReason(queryStructInfo,
-                            "View dimensions doesn't not cover the query 
dimensions",
-                            () -> String.format("mvExprToMvScanExprQueryBased 
is %s,\n queryGroupShuttledExpr is %s",
-                                    mvExprToMvScanExprQueryBased, 
queryGroupShuttledExpr));
-                    return null;
-                }
-                NamedExpression groupByExpression = rewrittenGroupByExpression 
instanceof NamedExpression
-                        ? (NamedExpression) rewrittenGroupByExpression : new 
Alias(rewrittenGroupByExpression);
-                finalOutputExpressions.add(groupByExpression);
-                finalGroupExpressions.add(groupByExpression);
+            Expression queryFunctionShuttled = 
ExpressionUtils.shuttleExpressionWithLineage(
+                    topExpression,
+                    queryTopPlan,
+                    queryStructInfo.getTableBitSet());
+            AggregateExpressionRewriteContext context = new 
AggregateExpressionRewriteContext(
+                    false, mvExprToMvScanExprQueryBased, queryTopPlan, 
queryStructInfo.getTableBitSet());
+            // queryFunctionShuttled maybe sum(column) + count(*), so need to 
use expression rewriter
+            Expression rollupedExpression = 
queryFunctionShuttled.accept(AGGREGATE_EXPRESSION_REWRITER,
+                    context);
+            if (!context.isValid()) {
+                materializationContext.recordFailReason(queryStructInfo,
+                        "Query function roll up fail",
+                        () -> String.format("queryFunctionShuttled = %s,\n 
mvExprToMvScanExprQueryBased = %s",
+                                queryFunctionShuttled, 
mvExprToMvScanExprQueryBased));
+                return null;
             }
+            finalOutputExpressions.add(new Alias(rollupedExpression));

Review Comment:
   final output should contains both aggregate function and group by dimension, 
this may lost group by dimension out put and change original agg output



-- 
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