github-actions[bot] commented on code in PR #67144:
URL: https://github.com/apache/doris/pull/67144#discussion_r4014845053
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/UnCorrelatedApplyAggregateFilter.java:
##########
@@ -117,7 +117,8 @@ public List<Rule> buildRules() {
PlanUtils.filterOrSelf(ImmutableSet.copyOf(unCorrelatedPredicate),
filter.child()));
return new LogicalApply<>(apply.getCorrelationSlot(),
apply.getSubqueryType(), apply.isNot(),
apply.getCompareExpr(), apply.getTypeCoercionExpr(),
- ExpressionUtils.optionalAnd(correlatedPredicate),
apply.getMarkJoinSlotReference(),
+
ExpressionUtils.mergeCorrelationFilter(apply.getCorrelationFilter(),
correlatedPredicate),
Review Comment:
[P1] Do not split aggregate groups for non-equality correlations
For `L.x=2` and `R={(r1=1,g=0),(r1=2,g=0)}`, `EXISTS (SELECT g FROM R WHERE
r1 <= L.x GROUP BY g HAVING count(*) = L.x)` is true: both rows form one `g=0`
group with count 2. This rule adds `r1` to the group keys while pulling up `r1
<= x`; after this merge preserves the earlier HAVING, the right side instead
has two count-1 groups, so neither satisfies `count_slot = x` and EXISTS
becomes false. `getUnCorrelatedExprs` accepts this inequality and EXISTS has no
rejection or later re-aggregation. Please preserve the correlation below the
aggregate, or reject non-equality cases where the added inner expression is not
functionally fixed, and add this count-based oracle.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/ExpressionUtils.java:
##########
@@ -227,6 +227,27 @@ public static Optional<Expression>
optionalAnd(Collection<Expression> collection
return optionalAnd(ImmutableList.copyOf(collection));
}
+ /**
+ * Merge newly extracted correlated predicates into an existing
correlation filter of an
+ * apply. the same apply can be rewritten multiple times (e.g. two nested
correlated
+ * filters), and each rule application pulls a new correlated predicate
into the apply.
+ * the existing correlation filter must be kept and AND-ed with the new
predicates,
+ * otherwise the previously extracted predicates are silently dropped and
never appear in
+ * the final join condition.
+ *
+ * @param existingCorrelationFilter the correlation filter accumulated on
the apply
+ * @param correlatedPredicates the newly extracted correlated predicates
+ * @return the merged correlation filter
+ */
+ public static Optional<Expression> mergeCorrelationFilter(
+ Optional<Expression> existingCorrelationFilter, List<Expression>
correlatedPredicates) {
+ List<Expression> allCorrelatedPredicates = new ArrayList<>();
+ existingCorrelationFilter.ifPresent(filter ->
allCorrelatedPredicates.addAll(
+ ExpressionUtils.extractConjunction(filter)));
+ allCorrelatedPredicates.addAll(correlatedPredicates);
Review Comment:
[P2] Preserve the WHERE/HAVING evaluation boundary for sensitive predicates
When an EXISTS subquery has a correlated HAVING above a correlated WHERE,
this merge moves both predicates into the post-aggregate join domain. For
example, with `L={(5)}` and `R={(2,0)}`, `WHERE r1 > l.x GROUP BY r1,r2 HAVING
assert_true(r2 = l.x, 'boom')` should produce no group and no error. After
decorrelation the right side first creates the `(2,0)` group, and the NLJ
evaluates `assert_true(0 = 5, 'boom')` on that candidate even though the
original WHERE removed the row. `AssertTrue` is a `NoneMovableFunction`, so
this changes observable semantics; reordering the conjuncts is insufficient
because each NLJ expression is evaluated over the candidate block before
masking. Please preserve the original stage for sensitive HAVING predicates (or
reject this shape) and add the empty-after-WHERE oracle.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/executor/Rewriter.java:
##########
@@ -467,6 +467,15 @@ public class Rewriter extends AbstractBatchJobExecutor {
),
// query rewrite support window, so add this rule here
custom(RuleType.AGG_SCALAR_SUBQUERY_TO_WINDOW_FUNCTION,
AggScalarSubQueryToWindowFunction::new),
+ // the analyzer may create redundant passthrough projects
around the aggregate
+ // (e.g. the group-by normalization projects of `GROUP
BY`), which block
+ // UnCorrelatedApplyAggregateFilter /
+ // PullUpCorrelatedFilterUnderApplyAggregateProject
+ // (they need Apply(Aggregate(Filter)) to pull up the
correlated predicate under
+ // the aggregate). eliminate them before unnesting so that
the correlated predicate
+ // under the aggregate is extracted into the apply's
correlationFilter instead of
+ // being silently dropped after apply-to-join.
+ custom(RuleType.ELIMINATE_UNNECESSARY_PROJECT,
EliminateUnnecessaryProject::new),
Review Comment:
[P2] Do not make this correctness fix disableable
`CustomRewriteJob` skips this pass when `disable_nereids_rules` contains
`ELIMINATE_UNNECESSARY_PROJECT`, which is an accepted non-privilege RuleType.
The analyzer project then remains in the new `qt_sql22` shape; as the comment
above notes, it blocks extraction of `r.r1 = l.x`, and apply-to-join silently
loses that predicate, returning TRUE where the oracle expects FALSE. Please
make the required wrapper handling part of the correlated-apply correctness
rewrite rather than a separately disableable generic cleanup, and add the same
oracle with this rule disabled.
--
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]
