[improvement](subquery) Improve mark join slot inference to eliminate redundant mark joins - #66482
Open
starocean999 wants to merge 6 commits into
Open
[improvement](subquery) Improve mark join slot inference to eliminate redundant mark joins#66482starocean999 wants to merge 6 commits into
starocean999 wants to merge 6 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29142 ms |
Contributor
TPC-DS: Total hot run time: 166247 ms |
Contributor
ClickBench: Total hot run time: 23.82 s |
starocean999
force-pushed
the
master_markjoin
branch
from
August 7, 2026 00:13
f86f0fe to
86ce3f1
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28673 ms |
Contributor
TPC-DS: Total hot run time: 165997 ms |
Contributor
ClickBench: Total hot run time: 23.73 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
starocean999
marked this pull request as ready for review
August 7, 2026 03:07
starocean999
requested review from
924060929,
englefly and
morrySnow
as code owners
August 7, 2026 03:07
morrySnow
changed the title
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
reviewed
Aug 7, 2026
| boolean sameResultForFalseAndNull = true; | ||
| boolean simplifiedForFalseAndNull = true; | ||
| for (int i = 0; i < loopCount; ++i) { | ||
| replaceMap.clear(); |
Contributor
There was a problem hiding this comment.
Optimization: missing early exit when both booleans are already false.
Once both sameResultForFalseAndNull and simplifiedForFalseAndNull have been set to false, no further iteration can flip them back to true. Adding if (!sameResultForFalseAndNull && !simplifiedForFalseAndNull) break; (or return Pair.of(false, false)) would short-circuit the remaining loop iterations.
In the worst case (4 mark slots → 27 inner iterations), the current code always runs all 27 iterations per target slot (108 iterations total for 4 targets), doing 4 FoldConstantRule.evaluate + 4 ExpressionUtils.replace calls per iteration (432 evaluations total). In many practical predicates both flags will be set to false by the first few iterations. An early exit could significantly reduce this.
| (predicate.collect(MarkJoinSlotReference.class::isInstance))); | ||
| int markSlotSize = markJoinSlotReferenceList.size(); | ||
| int maxMarkSlotCount = 4; | ||
| // if the conjunct has mark slot, and maximum 4 mark slots(for performance) |
Contributor
There was a problem hiding this comment.
Style: maxMarkSlotCount = 4 should be a private static final class-level constant.
As a local variable inside inferMarkSlotNotNullMap, the rationale for the limit (performance guard against 3^k combinatorial explosion) and the specific value 4 are harder to discover. Extracting it to a named constant at the class level, e.g. private static final int MAX_MARK_SLOT_COUNT_FOR_INFERENCE = 4, with a brief comment explaining the 3^k scaling, would make the performance contract explicit.
| public static Map<MarkJoinSlotReference, Pair<Boolean, Boolean>> inferMarkSlotNotNullMap( | ||
| Expression predicate, ExpressionRewriteContext ctx) { | ||
| ExpressionRewriteContext rewriteContext = new ExpressionRewriteContext(ctx.cascadesContext); | ||
| Expression simplifiedPredicate = TrySimplifyPredicateWithMarkJoinSlot.INSTANCE.rewrite(predicate, |
Contributor
There was a problem hiding this comment.
Minor: the ExpressionRewriteContext created here discards the plan from the caller's context.
ExpressionRewriteContext rewriteContext = new ExpressionRewriteContext(ctx.cascadesContext);
Expression simplifiedPredicate = TrySimplifyPredicateWithMarkJoinSlot.INSTANCE.rewrite(predicate,
rewriteContext);The passed-in ctx may have been constructed with a Plan (see simplifyConjunctWithMarkJoinSlot at SubqueryToApply.java line 157-158: plan == null ? new ExpressionRewriteContext(cascadesContext) : new ExpressionRewriteContext(plan, cascadesContext)). Here a fresh context is created from just cascadesContext, discarding the plan. While TrySimplifyPredicateWithMarkJoinSlot is a purely structural rewrite that doesn't depend on plan state today, the plan-less context is then also what flows into the fold-constant evaluation (via the original ctx). The two-context pattern is confusing — consider either reusing the caller's context directly for the simplify step, or adding a comment explaining why a fresh plan-less context is intentionally used here.
| : false; | ||
| Map<MarkJoinSlotReference, Pair<Boolean, Boolean>> markSlotsInfo; | ||
| if (join.getJoinType().isInnerOrCrossJoin() || join.getJoinType().isSemiJoin()) { | ||
| Pair<Expression, Map<MarkJoinSlotReference, Pair<Boolean, Boolean>>> simplifyResult = |
Contributor
There was a problem hiding this comment.
Question: why are outer/anti/asof joins excluded from mark-slot simplification?
The condition join.getJoinType().isInnerOrCrossJoin() || join.getJoinType().isSemiJoin() excludes LEFT/RIGHT OUTER, ANTI, and ASOF joins from the simplifyConjunctWithMarkJoinSlot call, falling back to an empty markSlotsInfo map. This means mark join elimination (info.second) and non-nullable mark inference (info.first) are both skipped for those join types.
For ANTI joins: the buildRules filter path (line 30-43) does call simplifyConjunctWithMarkJoinSlot unconditionally. But in the join-ON path, a NOT IN subquery in an ANTI join's ON clause would skip the simplification entirely. While this is conservative and correct, a brief comment explaining why these join types can't be simplified would help future readers (e.g., "outer joins preserve NULLs from the nullable side, making mark slot three-valued semantics observable").
…dant mark joins ### What problem does this PR solve? Problem Summary: The old `ExpressionUtils.canInferNotNullForMarkSlot` only returned a single boolean telling whether every mark join slot in a predicate could be treated as non-nullable (its null value could be replaced by false). That coarse, predicate-level answer limited how far the optimizer could go in eliminating redundant mark joins. This PR replaces it with `ExpressionUtils.inferMarkSlotNotNullMap`, which returns per-mark-slot information as a `Map<MarkJoinSlotReference, Pair<Boolean, Boolean>>`: - `Pair.first` is computed on the predicate simplified by `TrySimplifyPredicateWithMarkJoinSlot` (conjuncts without any mark slot in `And` are replaced by true, in `Or` by false): it is true when the simplified predicate taking false or null always evaluates to false or null, meaning the mark slot's null value can be replaced by false (the mark slot can be non-nullable). - `Pair.second` is computed on the original predicate: it is true when taking false or null always evaluates to false or null AND taking true always evaluates to true, meaning the predicate is equivalent to the mark slot being true. `SubqueryToApply` now consumes this map: when `Pair.second` is true, the mark slot conjunct is replaced by the true literal and the mark slot is eliminated from the `LogicalApply`, so an `IN` subquery used directly as a filter or join ON conjunct is unnested into a plain `LEFT_SEMI_JOIN`, and a `NOT IN` into a plain `NULL_AWARE_LEFT_ANTI_JOIN`, without materializing a mark column. The duplicated inference and replacement logic is extracted into `simplifyConjunctWithMarkJoinSlot`. Tests: - New `InferMarkSlotNotNullMapTest` covers And/Or/IsNull/IsNotNull/Nvl and multi-mark-slot predicates, including cases that distinguish the simplified predicate (pair.first) from the original predicate (pair.second). The old `CanInferNotNullForMarkSlotTest` is removed as it is superseded. - New `EliminateMarkJoinTest` verifies IN / NOT IN in join ON conditions are turned into plain semi / null-aware anti joins, while the mark join is kept when the mark is projected to the query output or when NULL semantics are observable (`is null`, `or`). ### Release note None ### Check List (For Author) - Test: Unit Test - Behavior changed: No - Does this need documentation: No
starocean999
force-pushed
the
master_markjoin
branch
from
August 7, 2026 07:38
86ce3f1 to
be24f2e
Compare
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29115 ms |
Contributor
TPC-DS: Total hot run time: 158615 ms |
Contributor
ClickBench: Total hot run time: 23.83 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #66435
Problem Summary:
The old
ExpressionUtils.canInferNotNullForMarkSlotonly returned a single boolean telling whether every mark join slot in a predicate could be treated as non-nullable (its null value could be replaced by false). That coarse, predicate-level answer limited how far the optimizer could go in eliminating redundant mark joins.This PR replaces it with
ExpressionUtils.inferMarkSlotNotNullMap, which returns per-mark-slot information as aMap<MarkJoinSlotReference, Pair<Boolean, Boolean>>:Pair.firstis computed on the predicate simplified byTrySimplifyPredicateWithMarkJoinSlot(conjuncts without any mark slot inAndare replaced by true, inOrby false): it is true when the simplified predicate taking false or null always evaluates to false or null, meaning the mark slot's null value can be replaced by false (the mark slot can be non-nullable).Pair.secondis computed on the original predicate: it is true when taking false or null always evaluates to false or null on the original predicate.SubqueryToApplynow consumes this map: whenPair.secondis true, the mark slot conjunct is replaced by the true literal and the mark slot is eliminated from theLogicalApply, so anINsubquery used directly as a filter or join ON conjunct is unnested into a plainLEFT_SEMI_JOIN, and aNOT INinto a plainNULL_AWARE_LEFT_ANTI_JOIN, without materializing a mark column. The duplicated inference and replacement logic is extracted intosimplifyConjunctWithMarkJoinSlot.Release note
None
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)