github-actions[bot] commented on code in PR #66445:
URL: https://github.com/apache/doris/pull/66445#discussion_r3747442228
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/MaterializedViewAggregateOnNoneAggregateRule.java:
##########
@@ -105,8 +105,8 @@ protected Pair<Map<BaseTableInfo, Set<String>>,
Map<BaseColInfo, Set<String>>> c
Pair<Map<BaseTableInfo, Set<String>>, Map<BaseColInfo, Set<String>>>
invalidPartitions
= super.calcInvalidPartitions(queryUsedBaseTablePartitionMap,
rewrittenPlan, cascadesContext,
materializationContext);
- if (PartitionCompensator.needUnionRewrite(invalidPartitions,
cascadesContext)) {
- // if query use some invalid partition in mv, bail out
+ if (PartitionCompensator.hasPartitionCompensation(invalidPartitions)) {
Review Comment:
This still rejects the removal-only case for the registered
aggregate-on-detail-MV path. For example, with a detail MV over `(d, k, v)`, a
query `GROUP BY k`, and a dropped base partition, the delegated calculation
returns a stale-MV removal entry with an empty base-compensation map. This
condition returns `null` before the common handler can simply remove that MV
partition below the query aggregate; no unsupported aggregate/base `UNION ALL`
is needed. Please keep rejecting pairs with a real base-table compensation
entry, but return removal-only pairs to the common handler, and add a
regression using a detail MV because the new test covers only
aggregate-on-aggregate rewrite.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java:
##########
@@ -375,17 +375,18 @@ protected List<Plan> doRewrite(StructInfo
queryStructInfo, CascadesContext casca
// if mv can not offer any partition for query, query
rewrite bail out to avoid cycle run
return rewriteResults;
}
- boolean partitionNeedUnion =
PartitionCompensator.needUnionRewrite(invalidPartitions, cascadesContext);
- boolean canUnionRewrite = canUnionRewrite(queryPlan,
- (AsyncMaterializationContext) materializationContext,
cascadesContext);
- if (partitionNeedUnion && !canUnionRewrite) {
+ boolean hasPartitionCompensation =
+
PartitionCompensator.hasPartitionCompensation(invalidPartitions);
+ boolean needBaseTableUnion =
!invalidPartitions.value().isEmpty();
+ if (needBaseTableUnion && !canUnionRewrite(queryPlan,
+ (AsyncMaterializationContext) materializationContext,
cascadesContext)) {
materializationContext.recordFailReason(queryStructInfo,
"need compensate union all, but can not, because
the query structInfo",
() -> String.format("mv partition info is %s, and
the query plan is %s",
mtmv.getMvPartitionInfo(),
queryPlan.treeString()));
return rewriteResults;
}
- if (partitionNeedUnion) {
+ if (hasPartitionCompensation) {
Review Comment:
In the removal-only case enabled by this change, `invalidPartitions.value()`
is empty, but we still call `addFilterOnTableScan`. An empty (but non-null)
partition map is not considered empty by `PredicateAddContext`, so this
traverses and deep-copies the full query, creates another cascades context, and
runs the whole-tree rewriter; line 399 then discards that result and only
applies `PartitionRemover`. This repeats for every matching MV alternative and
scales with the full query tree. Please handle the empty base-compensation map
first by applying `PartitionRemover` directly, and call `addFilterOnTableScan`
only when a base-table branch is actually needed.
--
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]
