github-actions[bot] commented on code in PR #67362:
URL: https://github.com/apache/doris/pull/67362#discussion_r3911463813
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -265,7 +270,102 @@ protected LogicalAggregate<Plan> aggregateRewriteByView(
queryAggregate.getSourceRepeat().get().getRepeatType(),
tempRewritedPlan);
return NormalizeRepeat.doNormalize(repeat);
}
- return new LogicalAggregate<>(finalGroupExpressions,
finalOutputExpressions, tempRewritedPlan);
+
+ // The rewritten aggregate should group by the query bottom
aggregate's group by expressions,
+ // and its output expressions should be the rewritten group by
expressions and the rolled up
+ // aggregate functions. The query top plan output expressions are
recomputed by a project above
+ // the rewritten aggregate, so the projection of a group by key in the
query top plan (such as
+ // `select cast(date_trunc(ts, 'day') as string) from t group by
date_trunc(ts, 'day')`) will not
+ // be wrongly treated as a group by key of the rewritten aggregate.
+ // The mapping from the query bottom aggregate output slot to the new
aggregate output expression
+ // is used to rewrite the query top plan output expressions to
reference the new aggregate output.
+ // The shuttled query bottom aggregate outputs are used as the map
keys and are passed to the
+ // rewriter directly, and the shuttled query top plan outputs restore
the projection expressions,
+ // so the whole top plan is traversed only twice instead of once per
output expression.
+ List<? extends Expression> shuttledBottomAggOutputs =
ExpressionUtils.shuttleExpressionWithLineage(
+ queryAggregate.getOutputExpressions(), queryTopPlan);
+ List<? extends Expression> shuttledTopPlanOutputs =
ExpressionUtils.shuttleExpressionWithLineage(
+ queryTopPlan.getOutput(), queryTopPlan);
+ Map<Expression, Expression> bottomAggOutputToNewExprMap = new
HashMap<>();
+ Set<Expression> queryGroupByExpressionSet = new
HashSet<>(queryGroupByExpressions);
+ List<NamedExpression> queryAggregateOutputs =
queryAggregate.getOutputExpressions();
+ for (int i = 0; i < queryAggregateOutputs.size(); i++) {
+ NamedExpression queryAggregateOutput =
queryAggregateOutputs.get(i);
+ Expression shuttledQueryAggregateOutput =
shuttledBottomAggOutputs.get(i);
+ if (queryGroupByExpressionSet.contains(queryAggregateOutput)) {
+ // if it is a group by expression, rewrite it to the new
aggregate group by key
+ Expression rewrittenGroupByExpression =
rewriteShuttledExpression(queryStructInfo,
+ shuttledQueryAggregateOutput,
mvExprToMvScanExprQueryBased, groupByMode,
+ materializationContext,
+ "View dimensions doesn't not cover the query
dimensions",
+ () -> String.format("mvExprToMvScanExprQueryBased is
%s,\n queryExpression is %s",
+ mvExprToMvScanExprQueryBased,
queryAggregateOutput));
+ if (rewrittenGroupByExpression == null) {
+ return null;
+ }
+ NamedExpression groupByOutput = rewrittenGroupByExpression
instanceof NamedExpression
+ ? (NamedExpression) rewrittenGroupByExpression : new
Alias(rewrittenGroupByExpression);
+ finalGroupExpressions.add(groupByOutput);
+ finalOutputExpressions.add(groupByOutput);
+ bottomAggOutputToNewExprMap.put(shuttledQueryAggregateOutput,
groupByOutput.toSlot());
+ } else {
+ // if it is an aggregate function, try to roll up and rewrite
+ Expression rewrittenFunction =
rewriteShuttledExpression(queryStructInfo,
+ shuttledQueryAggregateOutput,
mvExprToMvScanExprQueryBased, aggregateFunctionMode,
+ materializationContext,
+ "Query function roll up fail",
+ () -> String.format("queryExpression = %s,\n
mvExprToMvScanExprQueryBased = %s",
+ queryAggregateOutput,
mvExprToMvScanExprQueryBased));
+ if (rewrittenFunction == null) {
+ return null;
+ }
+ NamedExpression functionOutput = new Alias(rewrittenFunction);
+ finalOutputExpressions.add(functionOutput);
+ bottomAggOutputToNewExprMap.put(shuttledQueryAggregateOutput,
functionOutput.toSlot());
+ }
+ }
+
+ LogicalAggregate<Plan> rewrittenAggregate =
+ new LogicalAggregate<>(finalGroupExpressions,
finalOutputExpressions, tempRewritedPlan);
+
+ // rewrite the query top plan output expressions to reference the
rewritten aggregate output,
+ // the query top plan output slot is shuttled by lineage firstly to
restore the projection
+ // expression, so a projection of the group by key in the query top
plan can be recomputed
+ // by a project above the rewritten aggregate.
+ List<NamedExpression> topProjectExpressions = new ArrayList<>();
+ Set<ExprId> usedTopProjectExprIds = new HashSet<>();
+ for (Expression shuttledTopExpression : shuttledTopPlanOutputs) {
+ Expression replacedExpression =
ExpressionUtils.replace(shuttledTopExpression,
+ bottomAggOutputToNewExprMap);
+ if (replacedExpression instanceof NamedExpression
+ && usedTopProjectExprIds.add(((NamedExpression)
replacedExpression).getExprId())) {
+ topProjectExpressions.add((NamedExpression)
replacedExpression);
+ } else {
+ // Keep a distinct output expr id for each top project
position: multiple query top plan
+ // expressions can be rewritten to the same aggregate output
slot, for example
+ // `select sum(v) as s1, sum(v) as s2 from t group by a`.
Collapsing them would make the
+ // rewritten output set smaller than the query output set and
skip the plan normalization.
+ topProjectExpressions.add(new Alias(replacedExpression));
Review Comment:
A repeated bare output has one ExprId in the original plan, so forcing every
repeated rewritten slot to get a fresh alias flips the earlier cardinality bug
in the other direction. For example, the reduced normalized tree is:
```text
Project(k#1, k#1)
Aggregate(group=[k#1], output=[k#1])
```
Its `getOutputSet().size()` is 1. Both positions replace to the same MV slot
here; the first keeps that slot while the second gets a new Alias/ExprId, so
the rewritten output set has size 2. `MaterializedViewUtils.rewriteByRules`
then returns at its output-set-size guard before the required whole-tree
normalization and partition pruning, which can miscompute invalid-partition
compensation or leave the new aggregate unnormalized. This is distinct from the
existing separately aliased `s1`/`s2` thread, where the query starts with two
ExprIds. Please preserve the original top-output ExprId multiplicity, and add
an unaliased `SELECT k, k ... GROUP BY k` regression that makes the skipped
pass observable.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
