yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4366215346
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewAggregateRule.java:
##########
@@ -505,45 +504,73 @@ private Expression rewriteShuttledExpression(StructInfo
queryStructInfo, Express
* select b, count(*) from t1 group by b, after rewritten by materialized
view successfully
* If mv part partition is invalid, can not compensate union all, because
result is wrong after
* compensate union all.
+ *
+ * <p>The compensation unions two finished aggregates: what the
materialized view answers for the partitions
+ * it keeps, and what the base table answers for the partitions it does
not. It is right while no group is
+ * answerable by both, and only a grouping by the columns the view is
partitioned on, each of them by
+ * itself, keeps the two apart -- the view's partitions are disjoint in
that tuple, by value for a list
+ * partitioned view and by range for a range partitioned one. Grouping by
an expression over a partition
+ * column does not: {@code date_trunc(d, 'year')} answers one year out of
both branches, the view's part of
+ * it and the base table's, and the two rows come out beside each other.
Nor does a grouping set that
+ * leaves a partition column out -- the {@code ()} total of {@code
ROLLUP}/{@code CUBE}/{@code GROUPING
+ * SETS} puts every partition value in one group, which both branches
answer, and two partitions of a view
+ * partitioned on two columns can share one value of one of them.
*/
@Override
protected boolean canUnionRewrite(Plan queryPlan,
AsyncMaterializationContext context,
CascadesContext cascadesContext) {
- // Check query plan is contain the partition column
- // Query plan in the current rule must contain aggregate node, because
the rule pattern is
+ // The union is built over the whole query plan, so the outermost
aggregate is the one whose groups
+ // the two branches answer. The plan holds one, since the rule's
pattern requires it.
Optional<LogicalAggregate<Plan>> logicalAggregateOptional =
queryPlan.collectFirst(planTreeNode -> planTreeNode instanceof
LogicalAggregate);
if (!logicalAggregateOptional.isPresent()) {
return true;
}
- List<Expression> groupByExpressions =
logicalAggregateOptional.get().getGroupByExpressions();
+ LogicalAggregate<Plan> queryAggregate = logicalAggregateOptional.get();
+ List<Expression> groupByExpressions =
queryAggregate.getGroupByExpressions();
if (groupByExpressions.isEmpty()) {
// Scalar aggregate can not compensate union all
return false;
}
- MTMVPartitionInfo mvPartitionInfo =
context.getMtmv().getMvPartitionInfo();
- List<BaseColInfo> pctInfos = mvPartitionInfo.getPctInfos();
- boolean canUnionRewrite = false;
- // Check the query plan group by expression contains partition col or
not
- List<? extends Expression> groupByShuttledExpressions =
-
ExpressionUtils.shuttleExpressionWithLineage(groupByExpressions, queryPlan);
- for (Expression expression : groupByShuttledExpressions) {
- canUnionRewrite = !expression.collectToSet(expr -> {
- if (!(expr instanceof SlotReference) || !((SlotReference)
expr).isColumnFromTable()) {
- return false;
- }
- String columnName = ((SlotReference)
expr).getOriginalColumn().map(Column::getName).orElse(null);
- BaseTableInfo baseTableInfo = ((SlotReference)
expr).getOriginalTable().map(BaseTableInfo::new)
- .orElse(null);
- return pctInfos.stream().anyMatch(colInfo ->
colInfo.getTableInfo().equals(baseTableInfo)
- && colInfo.getColName().equals(columnName));
- }
- ).isEmpty();
- if (canUnionRewrite) {
- break;
+ List<BaseColInfo> pctInfos =
context.getMtmv().getMvPartitionInfo().getPctInfos();
+ Optional<LogicalRepeat<?>> logicalRepeatOptional =
queryAggregate.getSourceRepeat();
+ if (!logicalRepeatOptional.isPresent()) {
+ return groupingCoversPartitionColumns(groupByExpressions,
queryPlan, pctInfos);
+ }
+ // The group by list holds every grouping set's expressions beside the
grouping id, so it can not tell
+ // which set a partition column was read from; each set is asked on
its own instead.
+ for (List<Expression> groupingSet :
logicalRepeatOptional.get().getGroupingSets()) {
+ if (!groupingCoversPartitionColumns(groupingSet, queryPlan,
pctInfos)) {
+ return false;
}
}
- return canUnionRewrite;
+ return true;
+ }
+
+ /**
+ * Whether these grouping expressions hold every column the MV is
partitioned on, each of them by itself:
+ * an expression over a partition column puts several of its values in one
group, and one column of a
+ * multi-column partitioning leaves the other to vary within a group.
+ */
+ private static boolean groupingCoversPartitionColumns(List<Expression>
groupingExpressions, Plan queryPlan,
+ List<BaseColInfo> pctInfos) {
+ List<? extends Expression> groupingShuttledExpressions =
+
ExpressionUtils.shuttleExpressionWithLineage(groupingExpressions, queryPlan);
+ return pctInfos.stream().allMatch(colInfo ->
groupingShuttledExpressions.stream()
Review Comment:
Fixed in `2ce15a6c717`.
The closure no longer refuses a step onto a column the view is not
partitioned on:
```java
for (BaseColInfo equal : equalColumns.getOrDefault(column,
Collections.emptySet())) {
if (!groupedTogether.contains(equal)) {
unvisited.add(equal);
}
}
```
so `A.d = C.x` and `C.x = B.d` are walked as the one equality they imply,
and `GROUP BY a.d` reaches `B.d`
through `C.x`. The grouping still has to name one of the view's own
partition columns to start from, so a
grouping by `C.x` alone is not admitted -- the view holds no such column to
answer it with. What contributes an
equality is unchanged: two bare table columns tied by an inner join's
on-predicate or by a filter conjunct. A
chain that runs through an outer join contributes nothing, since a
null-supplied row does not satisfy the link
the next equality needs.
Measured on a local cluster both ways with the fixture `pct_join_transitive`
-- a view `PARTITION BY (a_d)` over
`pct_join_a a JOIN pct_join_c c ON a.d = c.x JOIN pct_join_b b ON c.x =
b.d`, daily range partitions, a 7-day
`partition_sync_limit`, and a row written into the day the view holds after
its refresh:
- with the previous version the rewrite declines the candidate: the plan is
the plain three-table join and the
fail info reads `View struct info is invalid, need compensate union all,
but can not, because the query
structInfo`, so the suite's own `explain` check fails there;
- with this change the plan is `VUNION` over the view plus the base branch
(`RBO...pct_join_mv3 chose`) and its
rows are what the same query gives with the rewrite off -- day offset 0:
11001, day offset 1: 10, which is
what the golden holds.
Validation: `mtmv_p0/test_mtmv_base_partition_read_scope` passes with the
new fixture, and with the
single-equality case of the previous round still beside it; the mv/mtmv FE
unit test classes -- 69 classes,
1075 tests -- pass. CI covers them again here.
--
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]
