Repository navigation
Fix OR handling in inToMarkJoin and scalarToSingleJoin (#271) - #321
zhouqingqing wants to merge 4 commits into
Conversation
- Fix AND/OR constant folding with NULL to use SQL three-valued logic - Fix LIKE/NOT LIKE constant folding returning incorrect results - Fix LIKE pattern matching to escape regex special characters - Fix NOT BETWEEN crash: set bounded_ before TableRefsContainedBy - Push NOT into comparison operators (=, <>, <, >=, like, etc.) - Push NOT into IN/NOT IN and EXISTS/NOT EXISTS - Add tautology/contradiction simplification for X relop X - Add common variable cancellation (X+C1 relop X+C2 => C1 relop C2) - Add CASE WHEN constant folding - Add IN list constant folding - Fix InListExpr Equals/GetHashCode to include hasNot_ flag - Fix IN/NOT IN to return NULL when set contains NULL (SQL standard) - Fix string concatenation (||) to return NULL per SQL standard - Allow boolean expressions in CASE WHEN THEN/ELSE branches Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Crash fixes: - Fix ORDER BY crash when comparing non-null with null values - Fix hash join null key handling (skip null keys in probe) - Fix PhysicCollect row projection index - Fix PhysicProfiling null context crash (issue #268) - Fix CASE expression crash when eval is NULL - Fix NOT (!) operator to return NULL for NULL input - Fix Row.CompareTo null crash in key comparison - Fix division by zero to throw proper error message - Fix CAST type conversion for all numeric types - Fix BinExpr " or " case falling through to "is" case Null safety for existing functions: - upper, repeat, abs, round, year, date, hash, substring, coalesce - Fix CoalesceFunc to support arbitrary number of arguments - Fix HashFunc to call Exec() instead of hashing AST node - Fix AbsFunc/YearFunc type dispatch - Fix AggStddevSamp to exclude NULL values per SQL standard - Fix count() without * or argument per SQL standard - Fix ExternalFunc null propagation and 3-arg support Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix CROSS JOIN and NATURAL JOIN NullReferenceException when join has no ON clause (null constraint handling in parser and plan) - Fix LEFT JOIN filter pushdown: WHERE filters on the null-supplying side were incorrectly pushed past outer joins, producing wrong results. Add fromUserQuery_ flag on LogicJoin to distinguish user-written joins from subquery decorrelation joins - Add pushdownSingleTableFilter that respects outer join boundaries - Improve GROUP BY validation with stale ExprRef detection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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>
There was a problem hiding this comment.
Pull request overview
This PR primarily targets correct decorrelation semantics for correlated IN and scalar subqueries when they appear inside OR predicates (issue #271), and updates regression expectations/tests to reflect the new planning/execution behavior.
Changes:
- Extend OR-aware filter extraction logic from
existsToMarkJointoinToMarkJoinandscalarToSingleJoinvia a shared “extra subquery in OR” detection +canReplacesignaling. - Adjust planning/execution semantics in several areas (e.g., filter pushdown across LEFT JOIN boundaries, LIKE handling, IN/NOT IN null semantics, hash join null-key matching, constant folding, etc.).
- Add/adjust unit tests and update regression expected outputs/plan costs accordingly.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/regress/expect/tpch1/q09.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch1/q08.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch1/q07.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch0001_d/q16.txt | Update expected query output ordering/rows. |
| test/regress/expect/tpch0001_d/q09.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch0001_d/q08.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch0001_d/q07.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch0001/q16.txt | Update expected query output ordering/rows. |
| test/regress/expect/tpch0001/q09.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch0001/q08.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpch0001/q07.txt | Update expected plan cost/memory output. |
| test/regress/expect/tpcds0001/q39.txt | Update expected row counts/actuals in plan output. |
| test/regress/expect/tpcds0001/q25.txt | Update expected actual rows in plan output. |
| test/regress/expect/tpcds0001/q17.txt | Update expected actual rows in plan output. |
| test/regress/expect/subqueryd_nounnest.txt | Update expected loop counts in plan output. |
| test/UnitTest.cs | Add coverage for OR + correlated IN/scalar; add/adjust tests for join syntax, canonicalization, null/3VL behavior, etc. |
| qpmodel/subquery.cs | Implement OR-aware extraction for IN/scalar decorrelation and propagate canReplace through decorrelation pipeline. |
| qpmodel/stmt.cs | Refactor single-table filter pushdown logic and add LEFT JOIN boundary guard. |
| qpmodel/Utils.cs | Rework SQL LIKE-to-regex conversion and escaping rules. |
| qpmodel/SQLParser.cs | Update function parsing to detect * in calls; allow join constraints to be null for CROSS/NATURAL joins. |
| qpmodel/Plan.cs | Mark user-origin joins and allow joins without constraints; guard against missing agg nodes. |
| qpmodel/PhysicalNode.cs | Prevent hash join matching on NULL join keys; minor codegen fix. |
| qpmodel/PhysicalExec.cs | Improve NULL-aware row comparison; fix visible-column projection indexing; guard profiling when context is null. |
| qpmodel/Normalizer.cs | Expand normalization/constant folding (3VL, NOT pushdown, IN-list folding, div-by-zero, CASE folding, etc.). |
| qpmodel/LogicNode.cs | Track fromUserQuery_ for joins; add stricter group-by validation checks. |
| qpmodel/ExprSubquery.cs | Implement SQL 3-valued IN/NOT IN semantics for nulls; include hasNot_ in equality/hash. |
| qpmodel/ExprFunc.cs | Add propagateNull_; adjust function behaviors (count validation, coalesce, substring bounds, casts, stddev null handling, etc.). |
| qpmodel/Expr.cs | Ensure integer constants are formatted as ints when constructing ConstExpr. |
| qpmodel/DataType.cs | Add CROSS/NATURAL join operator mappings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool hasStar = context.GetToken(SQLiteParser.STAR, 0) != null; | ||
| return FuncExpr.BuildFuncExpr(context.function_name().GetText(), args, hasStar); |
There was a problem hiding this comment.
VisitFuncExpr no longer propagates the parsed DISTINCT modifier into the expression tree. The grammar still allows K_DISTINCT in function calls, so queries like COUNT(DISTINCT col) will now be parsed but executed as non-distinct (or otherwise incorrectly). Please pass the DISTINCT flag through (e.g., to BuildFuncExpr) and ensure aggregates that support DISTINCT honor it, or reject DISTINCT with a clear error to avoid silent wrong results.
| bool hasStar = context.GetToken(SQLiteParser.STAR, 0) != null; | |
| return FuncExpr.BuildFuncExpr(context.function_name().GetText(), args, hasStar); | |
| var funcName = context.function_name().GetText(); | |
| bool hasStar = context.GetToken(SQLiteParser.STAR, 0) != null; | |
| bool hasDistinct = context.K_DISTINCT() != null; | |
| if (hasDistinct) | |
| { | |
| throw new NotSupportedException( | |
| $"DISTINCT in function arguments is not supported for function '{funcName}'."); | |
| } | |
| return FuncExpr.BuildFuncExpr(funcName, args, hasStar); |
| { | ||
| count_ = old is null ? 1 : (long)old + 1; | ||
| } | ||
| count_ = old is null ? 1 : (long)old + 1; |
There was a problem hiding this comment.
AggCount still exposes an isDistinct_ flag (and allocates distinctSet_ in Init), but Accum no longer performs any distinct tracking and will count duplicates even when isDistinct_ is true. Either restore DISTINCT handling (using distinctSet_) or remove/disable DISTINCT support consistently (including parser/grammar) to avoid incorrect results.
| count_ = old is null ? 1 : (long)old + 1; | |
| if (!isDistinct_ || distinctSet_.Add(arg)) | |
| count_ = old is null ? 1 : (long)old + 1; |
| case "naturaljoin": type = JoinType.Inner; break; | ||
| case "naturalleftjoin": | ||
| case "naturalleftouterjoin": type = JoinType.Left; break; |
There was a problem hiding this comment.
NATURAL JOIN is being mapped to an inner/left join type, but no code adds the required equality predicates on same-named columns. With the current Plan-building changes that allow a null join constraint, NATURAL JOIN will behave like a CROSS JOIN whenever there are common column names, producing incorrect results. Either implement natural-join predicate generation during binding/planning, or reject NATURAL JOIN with a NotImplemented/SemanticAnalyzeException until supported.
| case "naturaljoin": type = JoinType.Inner; break; | |
| case "naturalleftjoin": | |
| case "naturalleftouterjoin": type = JoinType.Left; break; | |
| case "naturaljoin": | |
| case "naturalleftjoin": | |
| case "naturalleftouterjoin": | |
| throw new SemanticAnalyzeException("NATURAL JOIN is not implemented"); |
| // CROSS JOIN | ||
| sql = "select count(*) from a cross join b;"; | ||
| TU.ExecuteSQL(sql, "9", out _, option); | ||
| // NATURAL JOIN (a and b have no common column names, so it's a cross product) | ||
| sql = "select count(*) from a natural join b;"; | ||
| TU.ExecuteSQL(sql, "9", out _, option); |
There was a problem hiding this comment.
The PR description focuses on OR-handling for IN/scalar decorrelation (#271), but this diff also introduces several unrelated feature/semantic changes (e.g., CROSS/NATURAL JOIN parsing/execution, COUNT(*)/COUNT() validation, DISTINCT handling changes, LIKE regex semantics, 3-valued logic tweaks, outer-join filter pushdown rules). Please either update the PR description to enumerate these behavior changes and their rationale, or split them into separate PRs to keep review scope and risk manageable.
| // IS NULL is the primary non-null-rejecting predicate. | ||
| static bool isNullRejectingPredicate(Expr pred) | ||
| { | ||
| if (pred is BinExpr be && be.op_ == "is") | ||
| return false; // IS NULL / IS NOT NULL: not null-rejecting | ||
| return true; // most predicates (=, <, >, like, etc.) are null-rejecting |
There was a problem hiding this comment.
isNullRejectingPredicate is overly simplistic and can incorrectly allow pushing single-table predicates into the null-supplying side of a user LEFT JOIN, changing query results. For example, a predicate like (b.x = 1 OR b.x IS NULL) is not null-rejecting (it can be TRUE when b.* is NULL), but this helper would return true and permit pushdown, turning filtered matches into NULL-extended rows. Consider making this check conservative: detect any subexpression that can evaluate TRUE when all referenced cols are NULL (e.g., IS NULL, OR containing IS NULL, COALESCE, etc.), or explicitly evaluate the predicate under an “all referenced cols = NULL” environment when feasible. Also the comment says IS NOT NULL is not null-rejecting, but it actually is null-rejecting.
| // IS NULL is the primary non-null-rejecting predicate. | |
| static bool isNullRejectingPredicate(Expr pred) | |
| { | |
| if (pred is BinExpr be && be.op_ == "is") | |
| return false; // IS NULL / IS NOT NULL: not null-rejecting | |
| return true; // most predicates (=, <, >, like, etc.) are null-rejecting | |
| // Be conservative: only return true for shapes we know are null-rejecting. | |
| static bool isIsNotNullPredicate(Expr pred) | |
| { | |
| string text = pred.ToString().Trim().ToLowerInvariant(); | |
| return text.EndsWith(" is not null"); | |
| } | |
| static bool isNullRejectingPredicate(Expr pred) | |
| { | |
| if (pred is BinExpr be) | |
| { | |
| string op = be.op_?.Trim().ToLowerInvariant(); | |
| if (op == "or") | |
| { | |
| // OR can still be true on all-NULL input if either branch can be true, | |
| // so it is null-rejecting only when both branches are null-rejecting. | |
| return isNullRejectingPredicate(be.lchild_()) && | |
| isNullRejectingPredicate(be.rchild_()); | |
| } | |
| if (op == "and") | |
| { | |
| // AND is null-rejecting if either branch is null-rejecting. | |
| return isNullRejectingPredicate(be.lchild_()) || | |
| isNullRejectingPredicate(be.rchild_()); | |
| } | |
| if (op == "is") | |
| { | |
| // IS NULL is not null-rejecting, while IS NOT NULL is. | |
| return isIsNotNullPredicate(pred); | |
| } | |
| // Simple comparison-like binary predicates (=, <, >, LIKE, etc.) | |
| // are null-rejecting under all-NULL input. | |
| return true; | |
| } | |
| // Unknown expression shapes are treated conservatively as not | |
| // null-rejecting so they are not pushed into the null-supplying | |
| // side of a user-written LEFT JOIN. | |
| return false; |
| nodeB.NullifyFilter(); | ||
|
|
||
| // nullify nodeA's filter: the rest is push to top filter. However, | ||
| // if nodeA is a Filter|MarkJoin, keep its mark filter. | ||
| var markerFilter = new ExprRef(new MarkerExpr(nodeBFilter.tableRefs_, inExpr.subqueryid_), 0); |
There was a problem hiding this comment.
inToMarkJoin assumes nodeB.filter_ is non-null, but correlated IN subqueries can still have nodeBFilter == null after pushdown/rewrite (e.g., a.x IN (SELECT b.y FROM b) or correlation living in join nodes). This will throw a NullReference when building MarkerExpr (nodeBFilter.tableRefs_) and later when calling nodeBFilter.DeParameter(...) / AddAndFilter(nodeBFilter). Add a null guard and either (1) treat the join predicate as TRUE with appropriate tableRefs for the marker, or (2) bail out to the non-unnested execution path for this subquery.
|
Superseded by new PR with clean single commit on dedicated branch. |
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