github-actions[bot] commented on code in PR #67362:
URL: https://github.com/apache/doris/pull/67362#discussion_r4014762096
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -183,57 +178,67 @@ 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()) {
+ // The group by/function classification of the query top plan
output expressions is only
+ // used by the repeat rewrite, so it is computed lazily inside
this branch to avoid paying
+ // the full plan lineage walk for every ordinary aggregate rewrite.
+ // 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 the query top plan expressions, the query top
plan output expressions
+ // are used as the repeat output expressions directly
+ 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);
Review Comment:
The `sourceRepeat` arm still loses distinct top-output identities before the
new reconstruction below. With an MV grouped by `k,h`, this query is reachable:
```sql
SELECT k AS k1, k AS k2, CAST(h AS STRING) AS hs, SUM(v)
FROM t GROUP BY GROUPING SETS ((k, h), (k));
```
Both `k` aliases lineage/rewrite to the same named MV slot, so this block
appends the same ExprId twice; the rewritten output set has three members while
the query has four. `rewriteByRules` therefore returns at its set-size guard.
`NormalizeRepeat.doNormalize` returns an Aggregate with `normalized=false`, so
the skipped whole-tree pass leaves the derived `Alias(CAST(mv_h))#hs` inside
the aggregate. Physical aggregate translation materializes `mv_h` and
aggregate-function outputs, but not `#hs`; the final positional project then
resolves `#hs` to null and reaches an NPE in `Expr.extractSlots`. This is
distinct from the existing duplicate-output threads because `sourceRepeat`
returns at line 270 before the non-Repeat ExprId-preserving logic. Please
preserve the original top-output ExprId equivalence classes in this arm (or
rebuild the original projection above a fully normalized Repeat aggregate) and
add a GROUPING SETS regression that proves named-MV selection and successful
execu
tion.
--
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]
