seawinde commented on code in PR #34274:
URL: https://github.com/apache/doris/pull/34274#discussion_r1591782528
##########
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));
}
// add project to guarantee group by column ref is slot reference,
// this is necessary because physical createHash will need
slotReference later
Review Comment:
Maybe add the following logic here is better, WDYT?
// if top plan doesn't use group by but bottom agg has group by, should also
check
if (!queryGroupByExpressions.isEmpty() &&
queryTopPlanGroupBySet.isEmpty()) {
// todo check as code in line 263 to 278
}
##########
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));
}
// add project to guarantee group by column ref is slot reference,
// this is necessary because physical createHash will need
slotReference later
Review Comment:
Maybe add the following logic here is better, WDYT?
```
// if top plan doesn't use group by but bottom agg has group by, should also
check
if (!queryGroupByExpressions.isEmpty() &&
queryTopPlanGroupBySet.isEmpty()) {
// todo check as code in line 263 to 278
}
```
--
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]
