github-actions[bot] commented on code in PR #66482:
URL: https://github.com/apache/doris/pull/66482#discussion_r3782608697
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/SubqueryToApply.java:
##########
@@ -118,20 +119,21 @@ public List<Rule> buildRules() {
ctx.statementContext,
shouldOutputMarkJoinSlot.get(i));
SubqueryContext context = new
SubqueryContext(subqueryExprs);
Expression conjunct =
replaceSubquery.replace(oldConjuncts.get(i), context);
- // TODO: The way to optimize null aware mark join is
not right.
- // remove it temporary until we refactor it.
- // ExpressionRewriteContext rewriteContext = new
ExpressionRewriteContext(ctx.cascadesContext);
- // boolean isMarkSlotNotNull =
conjunct.containsType(MarkJoinSlotReference.class)
- // ?
ExpressionUtils.canInferNotNullForMarkSlot(
- //
TrySimplifyPredicateWithMarkJoinSlot.INSTANCE.rewrite(conjunct,
- // rewriteContext),
rewriteContext)
- // : false;
- boolean isMarkSlotNotNull = false;
- Pair<LogicalPlan, Optional<Expression>> result =
subqueryToApply(subqueryExprs.stream()
- .collect(ImmutableList.toImmutableList()),
tmpPlan,
+ // the apply-stacking order inside the conjunct:
subqueryToApply builds the
+ // applies in this order, the first element being the
lowest
+ List<SubqueryExpr> orderedSubqueryExprs =
ImmutableList.copyOf(subqueryExprs);
+ Pair<Expression, Map<MarkJoinSlotReference,
Pair<Boolean, Boolean>>> simplifyResult =
+ simplifyConjunctWithMarkJoinSlot(conjunct,
filter, ctx.cascadesContext,
+ i, subqueryExprsList, null,
Review Comment:
[P2] Avoid building unused later-assertion domains
A bare top-level IN/EXISTS gets no `MarkJoinSlotReference`
(`ReplaceSubquery` substitutes TRUE), so `simplifyConjunctWithMarkJoinSlot`
takes its no-marker branch and never reads `extraEvaluationDomain`. This
argument is evaluated before that check, though, so every such conjunct still
walks all later subquery sets and may allocate throwaway Count/AssertTrue
trees. With N bare subquery conjuncts these scans sum to O(N^2) even though no
mark-slot inference runs. Please move this collection behind the marker check
(or pass it lazily/cache suffix sensitivity).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateEmptyRelation.java:
##########
@@ -231,7 +234,8 @@ private boolean bothChildrenEmpty(LogicalJoin<?, ?> join) {
}
private boolean canReplaceJoinByEmptyRelation(LogicalJoin<?, ?> join) {
- return !join.isMarkJoin() && ((join.getJoinType().isInnerJoin() ||
join.getJoinType().isAsofInnerJoin()
Review Comment:
[P1] Retaining this join still skips the probe-side check
This fence does not guarantee that the retained subtree executes. With a
lower correlated scalar and a removable higher empty IN, lowering can produce
`LeftSemi(o.k=e.k, Project(assert_true(count<=1), LeftOuterJoin(o, Agg(s))),
Empty(e))`. This method sees the generated scalar assertion and keeps the join,
and `FindHashConditionForJoin` makes the IN equality a hash condition. BE then
sets `short_circuit_for_probe` for the empty-build non-mark LEFT_SEMI join, so
it never requests that Project/probe chain; duplicate scalar rows stop raising
even though the logical subtree survived. Please keep the marker or introduce
an executable evaluation barrier for this shape (or make the runtime shortcut
honor such a contract), and add the corresponding expected-error regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateEmptyRelation.java:
##########
@@ -241,4 +245,32 @@ private boolean
canReplaceJoinByEmptyRelation(LogicalJoin<?, ?> join) {
|| (join.getJoinType().isAsofRightOuterJoin() && join.right()
instanceof EmptyRelation));
}
+ /**
+ * Whether replacing the join with an empty relation would discard a
side-effecting check that still has
+ * to run. The subtree must be preserved (i.e. the join must not be
eliminated) when it contains a scalar
+ * cardinality check ({@link LogicalAssertNumRows}) or an assert_true
({@link NoneMovableFunction}),
+ * otherwise the expected error is silently suppressed. For example
+ * {@code nvl(o.x = (select s.x from s) and exists(select 1 where false),
false)}: the higher empty EXISTS is
+ * lowered to a non-mark CROSS join with an empty relation, and replacing
it with empty would delete the lower
+ * scalar's {@link LogicalAssertNumRows} and return an empty result
instead of raising the
+ * "must return only 1 row" error. Volatile functions are intentionally
not checked here: their values are
+ * non-deterministic but not side-effecting, and they produce no rows
inside an empty join anyway.
+ */
+ private boolean containsSideEffect(Plan plan) {
+ if (plan instanceof LogicalAssertNumRows) {
+ return true;
+ }
Review Comment:
[P1] Do not preserve assertions from an empty join's ON domain
This recursive scan also treats an assertion that originated in the
candidate join's own ON clause as a mandatory child evaluation. For `big_t JOIN
(SELECT 1 WHERE FALSE) e ON assert_true(big_t.guard > 0, 'bad')`, there are no
row pairs, so the ON expression should never run and the result is empty.
`PushDownJoinOtherCondition` first moves that one-sided expression into the
left child; this check then retains the join, and the condition-free
CROSS/nested-loop path pulls the left child and raises `bad`. Please track the
expression's evaluation provenance (and make the pushdown honor
`NoneMovableFunction`) so only checks that were independently required below
the join block elimination. Add this empty-result versus error regression.
--
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]
