Plan analysis: the non-SARGable check reads which side and which table a function or conversion is on (#4518, #4519)

[erikdarlingdata]

Fixes #4518
Fixes #4519
Part of #4511

Why

The non-SARGable predicate check had several related faults. Each one produced a Non-SARGable Predicate warning for a predicate that can actually seek, or missed one that could not, and the accompanying advice then pointed at the wrong cause.

1. CONVERT_IMPLICIT was flagged whenever the predicate contained it, with no check of which side it converts. Data type precedence decides which side SQL Server converts — it converts the LOWER-precedence side — so a parameter compared to a wider-precedence column is converted up while the column itself stays untouched and seekable.
2. The function-on-column-side check split the WHOLE predicate at its first comparison operator. In a compound predicate that put every later comparison, column included, on the earlier function's side — a parameter-side function in the first conjunct made a real column reference later in the predicate look like it shared that side.
3. ISNULL/COALESCE shared the same unscoped check, so ISNULL(@p, 0) on the parameter side was reported as "wrapping a column" even though it is a runtime constant that seeks fine.
4. LIKE was not recognized as a comparison operator at all, so a pattern-side function before LIKE found no operator, fell back to the assume-the-worst default, and the pattern's function was reported as wrapping the column.
5. The check knew a column only by its dotted, bracketed name ([t].[col]), with no idea which table a name belonged to. An unaliased table variable column renders with no qualifier at all — the same shape as a parameter — so a function or conversion on it was never flagged. And a Nested Loops join passes a value into its inner scan as an outer reference; a function on that value wraps the outer input, not the scan's own column, but the old check counted every column-shaped name as the scan's own and flagged it anyway.

What changes

Ports PerformanceStudio 1c17b06 (erikdarlingdata/PerformanceStudio#436/#437), 6314f2c (erikdarlingdata/PerformanceStudio#556), 1016100 (erikdarlingdata/PerformanceStudio#561), 9a04629 (erikdarlingdata/PerformanceStudio#564) and 1374b75 into PlanAnalyzer.cs, matching PerformanceStudio main.

• ConvertImplicitWrapsColumn reads what is INSIDE the CONVERT_IMPLICIT call, using balanced-parenthesis argument extraction (the target type can carry its own parens — numeric(18,0), varchar(50)), instead of a bare text-contains check.
• IsFunctionOnColumnSide now reads only the one comparison a function belongs to: the text between the nearest AND/OR on either side, split at every parenthesis depth so a parenthesized OR group comes apart too, and never inside a string literal or a bracketed name. The comparison-operator pattern gains LIKE.
• The ISNULL/COALESCE check now walks every match and applies the same side-scoped check, instead of matching anywhere in the predicate.
• A new ScanIdentity (the scan's alias, its table, whether it's a table variable, and the bare outer references a Nested Loops ancestor passes it) is built once per scan and threaded through the checks above via a new IsColumnReference(text, identity) helper. It reads a dotted name's owner (the alias or table right before the column) and only counts it as the scan's own column when the owner matches. A bare, unqualified name on an unaliased table variable's own scan now also counts as a column, unless it is a parameter, an optimizer expression, or a name the identity lists as another table variable's outer reference.
• The owner comparison cleans a temp table's full tempdb name through the existing CleanTempTableName before comparing it against the scan's own (already-cleaned) name, so a predicate spelling the full name still matches.
• DetectNonSargablePattern is split from DetectNonSargablePredicate (a plan-node wrapper), so predicate shapes can be tested directly as ScalarString text.

PM has no AnalyzerConfig, so the cfg.IsRuleDisabled(N) guards from the PS commits are left out. PM keeps one PlanAnalyzer.cs file rather than PS's PlanAnalyzer.Detection.cs partial, so the new helpers land in the existing file next to the code they extend. ShowPlanParser.CleanTempTableName changes from private to internal so PlanAnalyzer can call it, matching PS.

Target-shape check: after the port, ConvertImplicitWrapsColumn(string, ScanIdentity? = null) and IsFunctionOnColumnSide(string, Match, ScanIdentity? = null) match PS main's PlanAnalyzer.Detection.cs apart from the cfg guards PM omits — same signatures, same helper call structure, same IsColumnReference seam.

Test plan

New file Darling/Darling.Tests/PlanSync4518Tests.cs (seven facts against PlanAnalyzer.DetectNonSargablePattern) and Darling/Darling.Tests/PlanSync4518EndToEndTests.cs (five facts through ShowPlanParser.Parse + PlanAnalyzer.Analyze) cover faults 1–4. The five end-to-end facts use only members that exist on dev and fail there at runtime (a standalone probe on origin/dev); the seven seam facts are compile-only on dev (DetectNonSargablePattern is new). Mutation: replacing the same-depth AND/OR scoping in IsFunctionOnColumnSide with the whole predicate fails the scoping fact (Actual: "Function call (UPPER) on column").

New file Darling/Darling.Tests/PlanSync4519Tests.cs covers fault 5, end to end through the real parser and analyzer on synthetic plan XML:

• UnaliasedTableVariableBareColumn_IsFlagged_EndToEnd: a table variable scan with predicate upper([col])=[@p] (bare, unqualified) — now flagged.
• OuterReferenceFunctionOnOuterSide_NotFlagged_EndToEnd: a Nested Loops inner scan predicate [t].[a]=upper([o].[x]), where [o].[x] is the outer reference — no longer flagged.
• TempTableFullTempdbName_OwnerComparisonMatches_EndToEnd / TempTableFullTempdbName_AnotherTempTable_NotFlagged_EndToEnd: a #temp table scan whose predicate names the table by its full tempdb (padded, hex-suffixed) name — the owner comparison cleans it and matches the scan's own column, and does not falsely match a different temp table's full name.

RED on dev (runtime): all three positive facts fail on origin/dev — UnaliasedTableVariableBareColumn_IsFlagged_EndToEnd (Assert.True: actual False — the old check never sees the bare column), OuterReferenceFunctionOnOuterSide_NotFlagged_EndToEnd (Assert.False: actual True — the old check counts the outer reference as the scan's own column), TempTableFullTempdbName_AnotherTempTable_NotFlagged_EndToEnd (Assert.False: actual True — the old check has no ownership concept at all, so any dotted name matches). Built and run in a detached worktree at origin/dev (0524634fa), since ShowPlanParser/PlanAnalyzer's public surface is unchanged there.

Mutation: temporarily made the dotted-name branch of IsColumnReference return true unconditionally (any dotted name counts, ownership ignored — the exact old-behavior fault this port fixes), rebuilt, reran PlanSync4519Tests: OuterReferenceFunctionOnOuterSide_NotFlagged_EndToEnd and TempTableFullTempdbName_AnotherTempTable_NotFlagged_EndToEnd both went RED with Assert.False() Failure: Actual: True. Reverted, rebuilt, reconfirmed green.

Run (after merging current dev): every PlanSync* class plus the plan-analysis classes passed, 0 failed. DarlingAnalysisPipelineTests and DarlingMcpPlanToolsLivePostgresTests need a live store and run in CI; before the merge, the same list ran 246/0 with a live store.

Build: Darling.Tests (Release, EnableWindowsTargeting=true) — 0 errors, 0 warnings. Lite.Tests (Windows-only, cannot execute on macOS) — 0 errors, 0 warnings.

Checked against PerformanceStudio main: ComparisonContaining, ExtractBalancedArguments, ConvertImplicitWrapsColumn, IsFunctionOnColumnSide, BuildScanIdentity and CollectBareOuterReferences are the same code; IsColumnReference and IsTableVariable differ only in comments and a '@'/"@" literal.

The temp-table facts use SQL Server's real tempdb name shape (#t + underscores + a 12-digit suffix).

CHANGELOG

SECTION: Fixed
ENTRY: - Plan analysis: the non-SARGable predicate check now knows which table a function, conversion or ISNULL/COALESCE call is wrapping, so it stops flagging a Nested Loops outer reference or a parameter-side conversion, recognizes LIKE as a comparison, and now also catches a function or conversion on an unaliased table variable's own column ([#4554]) - Each previously produced a Non-SARGable Predicate warning for a predicate that could actually seek (or, for the table-variable case, missed a real one), and pointed at the wrong cause.
REF: [#4554]: #4554