github-actions[bot] commented on code in PR #63763:
URL: https://github.com/apache/doris/pull/63763#discussion_r3593847339
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AggScalarSubQueryToWindowFunction.java:
##########
@@ -184,17 +203,26 @@ private boolean checkAggregate() {
}
/**
- * check inner scope only have one filter. and inner filter is a sub
collection of outer filter
+ * check inner filter conjuncts are a sub collection of outer filter.
+ * Also records which outer conjuncts were matched against inner conjuncts
so that rewrite()
+ * can force them below the window — they are part of the inner
aggregate's filter, not extra
+ * outer-only predicates.
+ * <p>
+ * PushDownFilterThroughProject can split a single LogicalFilter into
multiple filters
+ * (correlated predicates that reference slots not in a project's output
stay above,
+ * while non-correlated predicates can be pushed below). We collect
conjuncts from ALL
+ * inner filters rather than requiring exactly one.
*/
private boolean checkFilter(LogicalFilter<? extends Plan> outerFilter) {
List<LogicalFilter<Plan>> innerFilters = innerPlans.stream()
.filter(LogicalFilter.class::isInstance)
.map(p -> (LogicalFilter<Plan>)
p).collect(Collectors.toList());
- if (innerFilters.size() != 1) {
- return false;
- }
+ matchedInnerFilterConjuncts.clear();
Set<Expression> outerConjunctSet =
Sets.newHashSet(outerFilter.getConjuncts());
- Set<Expression> innerConjunctSet =
innerFilters.get(0).getConjuncts().stream()
+ // Collect conjuncts from ALL inner filter nodes —
PushDownFilterThroughProject
+ // may split correlated and non-correlated predicates into separate
filters.
+ Set<Expression> innerConjunctSet = innerFilters.stream()
Review Comment:
[P1] Do not let empty inner-filter sets bypass the shape proof
Removing the exactly-one-filter guard lets a zero-filter inner plan pass
without proving the rest of the shape, and there are two production failures
here.
First, `LogicalCTEConsumer` is accepted through `LogicalRelation`, but
identity, slot mapping, uniqueness selection, and shared-slot classification
only account for `CatalogRelation`. In `Filter(f.k=d.k, f.v>cnt) ->
Apply(CrossJoin(CTEConsumer f, unique Scan d), Aggregate(COUNT(d.k),
CTEConsumer f2))`, the checks see only `d`, move `f.k=d.k` below the window,
and change the witness from scalar `COUNT=2`/`2>2` false to window
`COUNT=1`/`2>1` true; this rule runs before `CTEInline`.
Second, a projected scalar subquery under an unrelated pushed filter can
produce `Project(cnt) -> Filter(f.k=d.k) -> Apply(..., Aggregate(COUNT(d.k),
Scan fact f2))`. The empty set passes this method, but that filter has no
aggregate-output conjunct, so `conjuncts.get(false)` is null and `rewrite()`
throws.
Please either reject zero-filter candidates or prove every supported
relation participates in identity/mapping/classification and require exactly
one aggregate-output comparison before rewriting. Add regressions for the
reused-CTE wrong-result case and the projected-subquery planning case.
<!-- codex-review-inline jmhpFe MAIN-1 -->
--
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]
