github-actions[bot] commented on code in PR #65393:
URL: https://github.com/apache/doris/pull/65393#discussion_r3664570705
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -444,6 +451,20 @@ public Void visitLogicalWindow(LogicalWindow<? extends
Plan> window, PartitionIn
return super.visitLogicalWindow(window, context);
}
+ @Override
+ public Void visitLogicalPartitionTopN(LogicalPartitionTopN<? extends
Plan> partitionTopN,
+ PartitionIncrementCheckContext context) {
+ if (!planOutputContainsPartitionColumnToCheck(partitionTopN,
context)) {
Review Comment:
[P1] Keep global Limit/TopN out of partition-local maintenance
A reachable rewritten plan is:
```
Limit(1)
Window(row_number() OVER (PARTITION BY p ORDER BY x))
PartitionTopN(keys=[p], hasGlobalLimit=true, limit=1)
Scan(p, x)
```
`PushDownLimit`/`PushDownTopNThroughWindow` leave the global operator above
the Window and set `hasGlobalLimit=true` on the inserted node. This visitor
only checks `keys=[p]`, so the PR now classifies the plan as PCT-capable;
previously the unsupported PartitionTopN stopped it. If only source partition A
is refreshed, the global limit is evaluated on A's restricted input while a
previously selected row in MV partition B is not removed, so the MV can contain
rows beyond the global result. Please reject `hasGlobalLimit=true` unless the
global operation is proven partition-local, and add a refresh test where the
global winner moves between MV partitions.
##########
regression-test/suites/nereids_rules_p0/mv/increment_create/cross_join_range_date_increment_create.groovy:
##########
@@ -336,12 +336,12 @@ suite("cross_join_range_date_increment_create",
"increment_create") {
def sql_all_list = [mv_sql_1, mv_sql_3, mv_sql_4, mv_sql_6, mv_sql_7,
mv_sql_8, mv_sql_9, mv_sql_10, mv_sql_11, mv_sql_12,
mv_sql_13, mv_sql_14, mv_sql_15, mv_sql_16, mv_sql_17,
mv_sql_18]
- def sql_increment_list = [mv_sql_1, mv_sql_3, mv_sql_4, mv_sql_6, mv_sql_7]
+ def sql_increment_list = [mv_sql_1, mv_sql_3, mv_sql_4, mv_sql_6,
mv_sql_7, mv_sql_13]
Review Comment:
[P2] Make the newly accepted refresh change the query result
`list_judgement` keeps the base tables between SQL cases and calls the same
`primary_tb_change` insert each time. By the time the newly accepted
`mv_sql_13` runs, that exact row has already been inserted by five earlier
accepted cases; this query groups every referenced key before evaluating its
windows, so one more duplicate produces the same source result. The right-table
pass repeats a grouped-away row too. This comparison can therefore pass without
exercising whether the newly supported nested-window refresh maintains a
result-changing update. Please reset/isolate state per case or use a fresh
group key, assert that the source result changed, then refresh and compare; the
other moved classification lists need the same treatment.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/PartitionIncrementMaintainer.java:
##########
@@ -476,25 +498,46 @@ private boolean checkWindowPartition(Expression
expression, PartitionIncrementCh
expression.collectToList(expressionTreeNode ->
expressionTreeNode instanceof WindowExpression);
for (Object windowExpressionObj : windowExpressions) {
WindowExpression windowExpression = (WindowExpression)
windowExpressionObj;
- List<Expression> partitionKeys =
windowExpression.getPartitionKeys();
- Set<Column> originalPartitionbyExprSet = new HashSet<>();
- partitionKeys.forEach(groupExpr -> {
- if (groupExpr instanceof SlotReference &&
groupExpr.isColumnFromTable()) {
- originalPartitionbyExprSet.add(((SlotReference)
groupExpr).getOriginalColumn().get());
- }
- });
- Set<SlotReference> contextPartitionColumnSet =
getPartitionColumnsToCheck(context);
- if (contextPartitionColumnSet.isEmpty()) {
- return false;
- }
- if (contextPartitionColumnSet.stream().noneMatch(
- partition ->
originalPartitionbyExprSet.contains(partition.getOriginalColumn().get()))) {
+ if
(!checkPartitionKeysContainPartitionToCheck(windowExpression.getPartitionKeys(),
context)) {
return false;
}
}
return true;
}
+ private boolean
checkPartitionKeysContainPartitionToCheck(List<Expression> partitionKeys,
+ PartitionIncrementCheckContext context) {
+ // Match partition keys by slot identity (exprId) instead of the
bare catalog Column,
+ // because Column.equals compares column attributes but not the
owning table: an unrelated
+ // input column with the same definition (e.g. l.p vs r.p) would
otherwise be accepted as
+ // the tracked partition key. The context already carries every
slot equal to the MV
+ // partition column via join equal-set propagation, so a
slot-level containment check keeps
+ // genuinely unrelated partition keys (no equality to the tracked
column) rejected.
+ Set<SlotReference> partitionKeySlotSet = new HashSet<>();
+ partitionKeys.forEach(partitionKey -> {
+ if (partitionKey instanceof SlotReference &&
partitionKey.isColumnFromTable()) {
+ partitionKeySlotSet.add((SlotReference) partitionKey);
+ }
+ });
+ Set<SlotReference> contextPartitionColumnSet =
getPartitionColumnsToCheck(context);
+ if (contextPartitionColumnSet.isEmpty()) {
+ return false;
+ }
+ return
contextPartitionColumnSet.stream().anyMatch(partitionKeySlotSet::contains);
Review Comment:
[P2] Resolve proven equal slots before comparing ExprIds
A reduced safe plan is:
```
Project(l.p#1, w#5)
Window(w OVER (PARTITION BY r.p#3))
InnerJoin(l.p#1 = r.p#3)
Scan l
Scan r
```
`visitLogicalWindow` calls this helper before descending into the join, so
the context still contains only `l.p#1`; `visitLogicalJoin` would add `r.p#3`
later. Exact containment therefore rejects this partition-local window. The
same regression occurs for a forwarding `p#1 AS p_alias#2`, whose equality is
already recorded by `LogicalProject.computeEqualSet` but whose ExprId differs.
This is the inverse of the existing unrelated-same-schema thread: here equality
is proven. Please expand lineage/equality classes from the child logical
properties before matching, while retaining distinct scan identities, and add
positive alias and inner-join-equality cases.
--
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]
