Repository navigation
Fix OR handling in inToMarkJoin and scalarToSingleJoin (#271) - #322
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes decorrelation/unnesting correctness for correlated IN and scalar subqueries that appear under OR in outer filters (issue #271), aligning their behavior with the existing existsToMarkJoin OR-aware handling.
Changes:
- Extend OR-aware filter extraction/removal to
inToMarkJoinandscalarToSingleJoinpaths viahasAnyExtraSubqueryExprInORand a propagatedcanReplaceflag. - Update decorrelation plumbing to pass
canReplacethroughoneSubqueryToJoininto the relevant conversion methods. - Add unit tests covering correlated
IN/NOT INinsideOR, and document/skip the known unsupported nested scalar-subquery-with-OR-inside-subquery-filter case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
qpmodel/subquery.cs |
Adds OR-aware handling and canReplace propagation for IN and scalar subquery decorrelation. |
test/UnitTest.cs |
Adds regression tests for correlated IN / NOT IN under OR and clarifies the unsupported nested scalar-OR case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool unusedCanReplace = false; | ||
| var newplan = djoinOnRightFilter(singleJoinNode, scalarExpr, ref unusedCanReplace); |
There was a problem hiding this comment.
Good observation. For the aggregation path, the scalar subquery is wrapped in an aggregate (e.g., SELECT max(b.i) FROM b WHERE ...), which guarantees a single row result. The OR-in-outer-filter scenario is handled by scalarToSingleJoin before dispatching to djoinOnRightAggregation, so canReplace is already set correctly at that level. The unusedCanReplace in djoinOnRightAggregation is intentional — the inner call only restructures the aggregation plan.
| LogicNode oneSubqueryToJoin(LogicNode planWithSubExpr, SubqueryExpr subexpr, ref bool canRepalce) | ||
| { | ||
| LogicNode oldplan = planWithSubExpr; | ||
| LogicNode newplan = null; | ||
|
|
||
| if (!subexpr.IsCorrelated()) | ||
| return planWithSubExpr; | ||
|
|
||
| switch (subexpr) | ||
| { | ||
| case ExistSubqueryExpr se: | ||
| newplan = existsToMarkJoin(planWithSubExpr, se, ref canRepalce); | ||
| break; | ||
| case ScalarSubqueryExpr ss: | ||
| newplan = scalarToSingleJoin(planWithSubExpr, ss); | ||
| newplan = scalarToSingleJoin(planWithSubExpr, ss, ref canRepalce); | ||
| break; | ||
| case InSubqueryExpr si: | ||
| newplan = inToMarkJoin(planWithSubExpr, si); | ||
| newplan = inToMarkJoin(planWithSubExpr, si, ref canRepalce); | ||
| break; |
There was a problem hiding this comment.
Agreed, but canRepalce is the existing spelling in the codebase (predates this PR). Renaming it would be a separate cleanup change — keeping it consistent with the existing code for now.
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #271 by extending OR-aware correlated-subquery decorrelation beyond existsToMarkJoin to also cover inToMarkJoin and scalarToSingleJoin, aiming to preserve correct semantics when correlated IN/scalar subqueries appear under OR conditions.
Changes:
- Add OR-aware filter extraction logic to correlated
INand scalar-subquery decorrelation paths (including acanReplacesignal to guide iteration state inDecorrelatePass). - Introduce/extend detection of OR conjuncts containing additional subqueries via
hasAnyExtraSubqueryExprInOR. - Add unit tests for correlated
IN/NOT INunder OR and annotate an existing unsupported nested scalar case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/UnitTest.cs | Adds regression tests covering correlated IN/NOT IN in OR and clarifies the unsupported nested scalar-subquery case. |
| qpmodel/subquery.cs | Propagates OR-aware extraction and canReplace signaling into inToMarkJoin and scalarToSingleJoin/djoinOnRightFilter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool unusedCanReplace = false; | ||
| var newplan = djoinOnRightFilter(singleJoinNode, scalarExpr, ref unusedCanReplace); |
| // if there is any (expr with @1 or @2), the root should be replaced | ||
| canReplace = andlist.Find(x => (x is LogicOrExpr) && hasAnyExtraSubqueryExprInOR(x, scalarExpr)) != null; | ||
|
|
||
| if (andlist.Count == 0 || canReplace) | ||
| nodeLeft.NullifyFilter(); | ||
| else |
Previously only existsToMarkJoin handled OR expressions correctly. When a correlated IN subquery or scalar subquery appeared inside an OR expression (e.g., "a1 IN (select ...) OR a2 > 1"), the decorrelation would not properly preserve OR semantics. Apply the same OR-aware filter extraction pattern from existsToMarkJoin to both inToMarkJoin (via extractCurINExprFromNodeAFilter) and scalarToSingleJoin (via djoinOnRightFilter): - Detect OR conjuncts containing extra subqueries - Set canReplace flag so the caller advances innerNode correctly - Preserve OR expressions for further unnesting Note: the nested case where OR appears *inside* a scalar subquery's filter alongside another correlated subquery (the original issue #271 example) remains unsupported during unnesting and falls back to nested-loop execution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c8bb76e to
62fed79
Compare
Summary
Fixes #271 — OR expressions were only handled in
existsToMarkJoinbut not ininToMarkJoinorscalarToSingleJoin. When a correlated IN/scalar subquery appeared inside an OR (e.g.,a1 IN (select ...) OR a2 > 1), decorrelation did not preserve OR semantics.existsToMarkJointoextractCurINExprFromNodeAFilteranddjoinOnRightFilterhasAnyExtraSubqueryExprInORcanReplaceflag so the caller advancesinnerNodecorrectlycanReplaceref fromoneSubqueryToJointo all three methodsNote: The nested case (OR inside a scalar subquery's filter alongside another correlated subquery — the original #271 example) remains unsupported during unnesting and falls back to nested-loop execution.
Test plan
🤖 Generated with Claude Code