github-actions[bot] commented on code in PR #67362:
URL: https://github.com/apache/doris/pull/67362#discussion_r3905142846
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -183,57 +183,59 @@ protected LogicalAggregate<Plan> aggregateRewriteByView(
materializationContext.getShuttledExprToScanExprMapping().keyPermute(viewToQuerySlotMapping)
.flattenMap().get(0);
Plan queryTopPlan = queryStructInfo.getTopPlan();
- for (Expression topExpression : queryTopPlan.getOutput()) {
- if (queryTopPlanFunctionSet.contains(topExpression)) {
- // if agg function, try to roll up and rewrite
- Expression rollupedExpression =
tryRewriteExpression(queryStructInfo, topExpression,
- mvExprToMvScanExprQueryBased, aggregateFunctionMode,
materializationContext,
- "Query function roll up fail",
- () -> String.format("queryExpression = %s,\n
mvExprToMvScanExprQueryBased = %s",
- topExpression, mvExprToMvScanExprQueryBased));
- if (rollupedExpression == null) {
- return null;
- }
- finalOutputExpressions.add(new Alias(rollupedExpression));
- } else {
- // if group by dimension, try to rewrite
- Expression rewrittenGroupByExpression =
tryRewriteExpression(queryStructInfo, topExpression,
- mvExprToMvScanExprQueryBased, groupByMode,
materializationContext,
- "View dimensions doesn't not cover the query
dimensions",
- () -> String.format("mvExprToMvScanExprQueryBased is
%s,\n queryExpression is %s",
- mvExprToMvScanExprQueryBased, topExpression));
- if (rewrittenGroupByExpression == null) {
- // group expr can not rewrite by view
- return null;
- }
- NamedExpression groupByExpression = rewrittenGroupByExpression
instanceof NamedExpression
- ? (NamedExpression) rewrittenGroupByExpression : new
Alias(rewrittenGroupByExpression);
- finalOutputExpressions.add(groupByExpression);
- finalGroupExpressions.add(groupByExpression);
- }
- }
LogicalAggregate<Plan> queryAggregate = queryTopPlanAndAggPair.value();
List<Expression> queryGroupByExpressions =
queryAggregate.getGroupByExpressions();
- // handle the scene that query top plan not use the group by in query
bottom aggregate
- if (needCompensateGroupBy(queryTopPlanGroupBySet,
queryGroupByExpressions)) {
- for (Expression expression : queryGroupByExpressions) {
- if (queryTopPlanGroupBySet.contains(expression)) {
- continue;
+ if (queryAggregate.getSourceRepeat().isPresent()) {
Review Comment:
This branch is now the only consumer of `queryTopPlanGroupBySet` and
`queryTopPlanFunctionSet`, but they are still computed unconditionally above
it. `topPlanSplitToGroupAndFunction` traverses the whole top plan once per
output, so every ordinary aggregate MV candidate pays that cost for data
immediately discarded. The non-Repeat loop then shuttles each bottom output
once for the map key and again inside `tryRewriteExpression`, followed by
another per-output shuttle for the top project. For wide aggregates and
multiple MV mappings these become several avoidable full-plan lineage walks,
quadratic in output width for wide plans. Please move the classification into
the Repeat branch and reuse or batch the non-Repeat lineage results.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -265,7 +267,87 @@ 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.
+ Map<Expression, Expression> bottomAggOutputToNewExprMap = new
HashMap<>();
+ Set<Expression> queryGroupByExpressionSet = new
HashSet<>(queryGroupByExpressions);
+ for (NamedExpression queryAggregateOutput :
queryAggregate.getOutputExpressions()) {
+ // The shuttled query bottom aggregate output is used as the map
key, so that the shuttled
+ // query top plan output expressions can be rewritten to reference
the new aggregate output.
+ Expression shuttledQueryAggregateOutput =
ExpressionUtils.shuttleExpressionWithLineage(
+ queryAggregateOutput, queryTopPlan);
+ if (queryGroupByExpressionSet.contains(queryAggregateOutput)) {
+ // if it is a group by expression, rewrite it to the new
aggregate group by key
+ Expression rewrittenGroupByExpression =
tryRewriteExpression(queryStructInfo,
+ queryAggregateOutput, 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 =
tryRewriteExpression(queryStructInfo, queryAggregateOutput,
+ 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<>();
+ for (Expression topExpression : queryTopPlan.getOutput()) {
+ Expression shuttledTopExpression =
ExpressionUtils.shuttleExpressionWithLineage(
+ topExpression, queryTopPlan);
+ Expression replacedExpression =
ExpressionUtils.replace(shuttledTopExpression,
+ bottomAggOutputToNewExprMap);
+ topProjectExpressions.add(replacedExpression instanceof
NamedExpression
Review Comment:
The map can collapse separately aliased duplicate outputs here. For `SELECT
SUM(v) AS s1, SUM(v) AS s2`, normalization keeps one bottom `SUM` slot but two
top ExprIds; both shuttled expressions now replace to the same
`functionOutput.toSlot()`, and this branch inserts that same Slot twice. The
rewritten project's output set therefore has size 1 while the query's has size
2, so `MaterializedViewUtils.rewriteByRules` returns at its output-set-size
guard before running the required whole-tree normalization and partition
pruning. In aggregate-on-detail mode this can even leave a derived grouping
Alias in an unnormalized aggregate. Please preserve a distinct named
output/ExprId for each top-project position and add a regression that makes the
skipped pass observable, such as a derived group expression in
aggregate-on-detail mode or partition pruning, rather than relying only on a
duplicate-`SUM` result.
--
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]
