Skip to content

Join OR Clause: skip the dynamic seek for a list of parameters (#558) - #559

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/558-join-or-parameter-in-list
Sep 24, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/558-join-or-parameter-in-list

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Summary

Fixes #558.

Rule 15 (Join OR Clause) checked only the operator shape: Nested Loops, Merge Interval, TopN Sort, Concatenation, and two or more Constant Scan branches. WHERE t.A IN (@p1, @p2) on an indexed column builds the same shape. It is a dynamic seek over the parameter values. So the rule told the user to rewrite a query with no join as a UNION ALL.

The rule now also reads what each lookup branch produces:

  • In a join OR, a branch produces a value from another input. That value is a column of the outer table, such as [Posts].[OwnerUserId] as [p].[OwnerUserId]. It can also be an expression that the outer input computes, such as [Expr1002].
  • In the dynamic seek for a list of parameters, every branch produces only parameters, variables and literals, such as [@p1] and (62). Functions of them, such as LikeRangeStart([@a]), are the same case.

The rule fires when at least one branch reads another input. When a branch has no values to read, the rule keeps the warning, as before.

The issue suggested a check for column names only. That check loses a real warning. An OR join on expressions of the outer columns (ON t.A = o.X + 1 OR t.A = o.Y + 1) produces [Expr1002] and [Expr1003], and no column names. So the new check counts every name that is not a parameter or a variable as another input.

Checked on SQL Server 2022

I captured actual plans for nine query shapes on a local SQL Server 2022 instance (16.0.4255.1). Then I ran planview analyze on each plan, before and after the change.

Query shape Before After
WHERE t.A IN (@p1, @p2), the query in the issue Join OR Clause no warning
WHERE t.A IN (@a, @b) with local variables Join OR Clause no warning
WHERE t.A IN (@p1, ABS(@p2)) Join OR Clause no warning
WHERE t.V LIKE @a OR t.V LIKE @b Join OR Clause no Join OR Clause
ON t.A = o.X OR t.A = o.Y Join OR Clause Join OR Clause
ON t.A = o.X OR t.A = @p Join OR Clause Join OR Clause
ON t.A = o.X + 1 OR t.A = o.Y + 1 Join OR Clause Join OR Clause
An OR join to a table variable Join OR Clause Join OR Clause
An OR join on computed columns of a derived table Join OR Clause Join OR Clause

The LIKE query still gets an Expensive Operator warning, before and after. That warning comes from rule 35, which this change does not touch.

Tests

  • in_list_dynamic_seek_plan.sqlplan is the actual plan from the issue. It gets no Join OR Clause warning. planview analyze now reports no warnings on it.
  • join_or_mixed_parameter_plan.sqlplan and join_or_expression_plan.sqlplan are real OR joins from SQL Server 2022. Each one keeps its warning.
  • Raw-string tests on ReadsAnotherInput cover parameters, variables, functions of parameters and a user-defined function. They also cover outer columns, [Expr] names, a table variable column, and a bracket inside a string literal.
  • A mutation check applied five wrong versions of the code. The test written for each one caught it:
    • No value check (the old behavior)
    • All in place of Any
    • Column names only, as the issue suggested
    • No string-literal skip
    • Function names read as names
  • WarningBaseline.txt changes only by the sections for the three new fixtures. No existing fixture gains or loses a warning.
  • ComparisonBaseline.txt adds 8 pairings, and each one includes a new fixture. It removes 2 pairings, which now go through a new fixture in name order. The text of every other pairing is unchanged.
  • Full Core test suite: 736 tests, 735 passed, 1 skipped, 0 failed. The Web project builds.

Not in this PR

In ComparisonBaseline.txt, the statement text of the new fixture is (10 int, 20 int)SELECT .... That is a different bug. Parameter substitution puts values into the declaration list at the start of a cached sp_executesql statement. A separate PR fixes it and updates these lines.

🤖 Generated with Claude Code

https://claude.ai/code/session_017ZVrq8tpA2DBPEFEqahFK6

Rule 15 matched the operator shape alone. WHERE t.A IN (@p1, @p2) on an
indexed column builds the same Nested Loops / Merge Interval / TopN Sort /
Concatenation / Constant Scan chain as a dynamic seek over the parameter
values, so a query with no join was told to rewrite as UNION ALL (#558).

The rule now also reads what each lookup branch produces, and fires only
when at least one branch takes a value from another input: an outer column,
or an expression the outer input computed ([Expr1002] for ON t.A = o.X + 1
OR ...). Branches of parameters, variables, literals and functions of them
are a parameter list. A branch with nothing to read keeps the warning.

Fixtures: the reporter's plan, plus a mixed column/parameter OR join and an
OR join on outer expressions, both captured on SQL Server 2022.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017ZVrq8tpA2DBPEFEqahFK6
@claude

claude Bot commented Sep 24, 2026

Copy link
Copy Markdown

Reviewed the Rule 15 change (PlanAnalyzer.Detection.cs, PlanAnalyzer.Node.cs, PlanAnalyzer.cs).

Traced ReadsAnotherInput/BracketedNameRegex against all the raw-string test cases (parameter-only, function-of-parameter, table-variable column, [Expr] names, dotted multi-part names, bracket-in-string-literal) plus the three new .sqlplan fixtures — the logic checks out: string literals are consumed whole before bracket scanning can misfire on them, function-call names are correctly excluded via the call capture, and dotted chains ([db].[schema].[T].[c]) are matched as a single name so the "not a parameter" check doesn't get fooled by the first segment. The Any(LookupReadsAnotherInput) semantics correctly keep the warning for mixed branches (one column branch + one parameter branch), matching the PR's stated goal of not losing real join-OR detections.

No null-safety issues (string.IsNullOrEmpty guards before regex use, no unguarded PhysicalOp/DefinedValues access), no injection concerns (these strings only feed warning text, never generated T-SQL), and no repo-convention violations — all three touched files are already linked into PlanViewer.Web.csproj, so no linked-file gap. The regex itself ((?:[^\]]|\]\])*-style alternation) is linear, not backtracking-prone, despite operating on untrusted plan XML.

Nothing to flag. The mutation-style test coverage described in the PR body (five deliberately-wrong implementations, each caught by a specific test) matches what I'd want to see for a detection-shape change like this.

@erikdarlingdata
erikdarlingdata merged commit a2f2d92 into dev Sep 24, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/558-join-or-parameter-in-list branch September 24, 2026 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant