From 6314f2c0934a6ec4b1de19b46f5c1bbd85879070 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 23 Sep 2026 17:55:16 -0400 Subject: [PATCH] Read only the function's own comparison in the non-SARGable check IsFunctionOnColumnSide split the whole predicate at its first comparison operator. In a compound predicate that put every later comparison, column and all, on the function's side, so [t].[A]=CONVERT_IMPLICIT(int,[@1],0) AND [t].[B]=CONVERT(tinyint,[@2],0) reported a conversion of a parameter as a function on a column. The same mistake fired on the everyday date range [d]>=dateadd(day,(-7),getdate()) AND [d] Claude-Session: https://claude.ai/code/session_01E9G1hWs6Nk4h4U2gn7TSy5 --- .../Services/PlanAnalyzer.Detection.cs | 88 ++++++++++-- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 19 +++ .../ComparisonBaseline.txt | 43 +++++- .../PlanAnalyzerTests.cs | 133 ++++++++++++++++++ ...n_sargable_compound_predicate_plan.sqlplan | 2 + .../PlanViewer.Core.Tests/WarningBaseline.txt | 3 + 6 files changed, 267 insertions(+), 21 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/Plans/non_sargable_compound_predicate_plan.sqlplan diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs index dbd1b21..28f5d95 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs @@ -158,8 +158,20 @@ private static bool IsScanOperator(PlanNode node) if (!IsRowstoreScan(node)) return null; - var predicate = node.Predicate; + return DetectNonSargablePattern(node.Predicate); + } + /// + /// The pattern half of : which non-SARGable shape, if + /// any, a predicate ScalarString has. + /// + /// Internal so predicate shapes can be tested as raw strings. The shapes that matter + /// (compound AND/OR predicates, date ranges, parenthesized groups, AND inside a literal or a + /// bracketed name) outnumber any sensible set of plan fixtures, and every one of them is + /// decided entirely in this method and the helpers it calls. + /// + internal static string? DetectNonSargablePattern(string predicate) + { // CASE expression in predicate — check first because CASE bodies // often contain CONVERT_IMPLICIT that isn't the root cause if (CaseInPredicateRegex.IsMatch(predicate)) @@ -170,9 +182,15 @@ private static bool IsScanOperator(PlanNode node) if (ConvertImplicitWrapsColumn(predicate)) return "Implicit conversion (CONVERT_IMPLICIT)"; - // ISNULL / COALESCE wrapping column - if (Regex.IsMatch(predicate, @"\b(isnull|coalesce)\s*\(", RegexOptions.IgnoreCase)) - return "ISNULL/COALESCE wrapping column"; + // ISNULL / COALESCE wrapping column — on the column side only. ISNULL(@p, 0) on the + // parameter side is a runtime constant and seeks fine; flagging it contradicted this + // warning's own "wrapping a column" message. col = ISNULL(@p, col) is still caught, + // because the column sits inside the function, on its side of the comparison. + foreach (Match isnullMatch in IsnullCoalesceRegex.Matches(predicate)) + { + if (IsFunctionOnColumnSide(predicate, isnullMatch)) + return "ISNULL/COALESCE wrapping column"; + } // Common function calls on columns — but only if the function wraps a column, // not a parameter/variable. Split on comparison operators to check which side @@ -261,31 +279,71 @@ internal static bool ConvertImplicitWrapsColumn(string predicate) /// Checks whether a function call in a predicate is on the column side of the comparison. /// Predicate ScalarStrings look like: [db].[schema].[table].[col]>dateadd(day,(0),[@var]) /// If the function is only on the parameter/literal side, it's still SARGable. + /// + /// Only the function's own comparison is read (#556). A compound predicate is + /// several comparisons joined by AND/OR, and the function belongs to exactly one of them. + /// Splitting the whole predicate at its FIRST operator instead put every later comparison, + /// column and all, on the function's side: in [t].[A]=[@1] AND [t].[B]=CONVERT(tinyint,[@2],0) + /// the CONVERT looked like it shared a side with [t].[B], and so did the dateadd in the + /// everyday range [t].[d]>=dateadd(day,(-7),getdate()) AND [t].[d]<getdate(). /// private static bool IsFunctionOnColumnSide(string predicate, Match funcMatch) { - // Find the comparison operator that splits the predicate into left/right sides. - // Operators in ScalarString: >=, <=, <>, >, <, = - var compMatch = Regex.Match(predicate, @"(?])([<>=!]{1,2})(?![<>=])"); + var comparison = ComparisonContaining(predicate, funcMatch.Index, out var offset); + + var compMatch = ComparisonOperatorRegex.Match(comparison); if (!compMatch.Success) return true; // No comparison found — can't determine side, assume worst case var compPos = compMatch.Index; - var funcPos = funcMatch.Index; - - // Determine which side the function is on - var funcSide = funcPos < compPos ? "left" : "right"; + var funcPos = funcMatch.Index - offset; - // Check if that side also contains a column reference [...].[...].[...] - string side = funcSide == "left" - ? predicate[..compPos] - : predicate[(compPos + compMatch.Length)..]; + // The side of this comparison the function is on, and whether a column shares it + string side = funcPos < compPos + ? comparison[..compPos] + : comparison[(compPos + compMatch.Length)..]; // Same column-vs-variable distinction ConvertImplicitWrapsColumn needs, so it shares the // one regex rather than keeping a second copy of the pattern in sync by hand. return ColumnReferenceRegex.IsMatch(side); } + /// + /// The single comparison around : the text between the nearest + /// AND/OR before it and the nearest after it. is where that text + /// starts in , so positions can be translated into it. + /// + /// Operators are split on at every depth, not just the top level: a parenthesized group + /// like [t].[A]=(1) AND ([t].[B]=f([@p]) OR [t].[C]=(3)) has to come apart into its + /// three comparisons, or the group would be read as one. The leftover grouping parentheses + /// cannot move a comparison operator or add a column, so they are harmless. No function in a + /// ScalarString takes AND/OR inside its arguments; CASE does, and it is caught earlier. + /// + private static string ComparisonContaining(string predicate, int position, out int offset) + { + var start = 0; + var end = predicate.Length; + + foreach (Match match in LogicalOperatorRegex.Matches(predicate)) + { + if (!match.Groups[1].Success) + continue; // a string literal or bracketed name, skipped whole + + if (match.Index + match.Length <= position) + { + start = match.Index + match.Length; + } + else + { + end = match.Index; + break; + } + } + + offset = start; + return predicate[start..end]; + } + /// /// Verifies the OR expansion chain walking up from a Concatenation node: /// Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index aea2bef..dd4e012 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -39,6 +39,25 @@ one failed the match and the Non-SARGable warning silently vanished. A bare [@p] @"\[[^\]]+\]\.\[", RegexOptions.Compiled); + /* The operator a comparison turns on in a ScalarString: >=, <=, <>, !=, >, <, = or like. + Without like, [col] like upper([@p]) had no operator at all, fell to the assume-the-worst + default, and a function on the pattern was reported as a function on the column. */ + private static readonly Regex ComparisonOperatorRegex = new( + @"(?])([<>=!]{1,2})(?![<>=])|\s(like)\s", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + + /* What joins one comparison to the next in a compound predicate. String literals and + bracketed identifiers are matched first, so an AND inside one of them (N'Tom AND Jerry', + [Terms and Conditions]) is consumed whole and never reaches the capture group. Only a + Groups[1] match is a real operator. */ + private static readonly Regex LogicalOperatorRegex = new( + @"'(?:[^']|'')*'|\[(?:[^\]]|\]\])*\]|\s(AND|OR)\s", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + + private static readonly Regex IsnullCoalesceRegex = new( + @"\b(isnull|coalesce)\s*\(", + RegexOptions.IgnoreCase | RegexOptions.Compiled); + public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null, ServerMetadata? serverMetadata = null) => AnalyzeCancellable(plan, config, serverMetadata, CancellationToken.None); diff --git a/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt b/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt index e6d8537..83eaecb 100644 --- a/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt +++ b/tests/PlanViewer.Core.Tests/ComparisonBaseline.txt @@ -641,6 +641,21 @@ UPDATE [dbo].[Users] set [Age] = 138 WHERE [Id]=22656 DOP: 1 -> 1 +##### non_sargable_compound_predicate_plan.sqlplan vs non_sargable_compound_predicate_plan.sqlplan +=== Plan Comparison === +Plan A: non_sargable_compound_predicate_plan.sqlplan +Plan B: non_sargable_compound_predicate_plan.sqlplan + +--- Statement 1 --- +(52 tinyint,2 int)SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2) + + Estimated cost: 0.0033 -> 0.0033 (0.0% costlier) + Estimated rows: 1 -> 1 (0.0% more) + Runtime: 8ms -> 8ms (0.0% slower) + CPU time: 7ms -> 7ms (0.0% slower) + DOP: 1 -> 1 + + ##### non_sargable_function_plan.sqlplan vs non_sargable_function_plan.sqlplan === Plan Comparison === Plan A: non_sargable_function_plan.sqlplan @@ -1765,19 +1780,35 @@ INSERT INTO [dbo].[Users]([AboutMe],[Age],[CreationDate],[DisplayName],[DownVote DOP: 1 -> 1 -##### multi_index_update_plan.sqlplan vs non_sargable_function_plan.sqlplan +##### multi_index_update_plan.sqlplan vs non_sargable_compound_predicate_plan.sqlplan === Plan Comparison === Plan A: multi_index_update_plan.sqlplan -Plan B: non_sargable_function_plan.sqlplan +Plan B: non_sargable_compound_predicate_plan.sqlplan --- Statement 1 --- UPDATE [dbo].[Users] set [Age] = 138 WHERE [Id]=22656 - Estimated cost: 0.0633 -> 3,097.52 (9,999% costlier) + Estimated cost: 0.0633 -> 0.0033 (94.8% cheaper) + Estimated rows: 1 -> 1 (0.0% more) + Runtime: 1ms -> 8ms (700% slower) + CPU time: 1ms -> 7ms (600% slower) + Logical reads: 3 -> 0 (eliminated) + DOP: 1 -> 1 + + +##### non_sargable_compound_predicate_plan.sqlplan vs non_sargable_function_plan.sqlplan +=== Plan Comparison === +Plan A: non_sargable_compound_predicate_plan.sqlplan +Plan B: non_sargable_function_plan.sqlplan + +--- Statement 1 --- +(52 tinyint,2 int)SELECT COUNT(*) FROM [dbo].[T] [t] WHERE [t].[A]=52 AND [t].[B]=CONVERT([tinyint],2) + + Estimated cost: 0.0033 -> 3,097.52 (9,999% costlier) Estimated rows: 1 -> 1 (0.0% more) - Runtime: 1ms -> 725ms (9,999% slower) - CPU time: 1ms -> 5.8s (9,999% slower) - Logical reads: 3 -> 4,226,624 (9,999% more) + Runtime: 8ms -> 725ms (8,962% slower) + CPU time: 7ms -> 5.8s (9,999% slower) + Logical reads: 0 -> 4,226,624 (new) Memory grant: 0.0 MB -> 24.2 MB (new) DOP: 1 -> 8 Warnings: 0 -> 5 (5 new) diff --git a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs index bf98c4b..b10ac08 100644 --- a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs @@ -335,6 +335,139 @@ public void Rule12f_NonSargable_TableVariableParameterSideConversion_IsNotFlagge "[@tv].[col]=CONVERT_IMPLICIT(nvarchar(40),[@p],0)")); } + // --------------------------------------------------------------- + // Rule 12: Non-SARGable Predicate — compound predicates (#556) + // --------------------------------------------------------------- + + /// + /// #556: the side check split the WHOLE predicate at its first comparison operator. In + /// [t].[A]=CONVERT_IMPLICIT(int,[@1],0) AND [t].[B]=CONVERT(tinyint,[@2],0) that operator + /// sits after [t].[A], so the second conjunct, [t].[B] included, landed on the CONVERT's side, + /// and a conversion of a parameter was reported as a function on a column. Both columns here are + /// compared bare. The fixture is the reporter's own SQL Server 2022 actual plan: an + /// auto-parameterized query on a table with no index on A or B, which is why it scans. Rule 11 + /// still reports that scan, which is true and is the actionable half. + /// + [Fact] + public void Rule12g_NonSargable_ParameterSideConvertAfterAnotherComparison_NotFlagged() + { + var plan = PlanTestHelper.LoadAndAnalyze("non_sargable_compound_predicate_plan.sqlplan"); + + Assert.Empty(PlanTestHelper.WarningsOfType(plan, "Non-SARGable Predicate")); + Assert.NotEmpty(PlanTestHelper.WarningsOfType(plan, "Scan With Predicate")); + } + + /// + /// The everyday shape of the same bug, and likely the most common one in the field: a date + /// range with dateadd() on the parameter side of the lower bound. Under the old split the + /// upper bound's column sat on the dateadd's side, and a perfectly SARGable range was told to + /// "remove the function from the column side". + /// + [Fact] + public void Rule12g_NonSargable_DateRangeWithParameterSideDateadd_NotFlagged() + { + Assert.Null(PlanAnalyzer.DetectNonSargablePattern( + "[db].[dbo].[Posts].[CreationDate] as [p].[CreationDate]>=dateadd(day,(-7),getdate()) " + + "AND [db].[dbo].[Posts].[CreationDate] as [p].[CreationDate] + /// The fix must narrow the side check, not blunt it: a function that really does wrap a column + /// is still caught when it sits in a later comparison of a compound predicate. + /// + [Fact] + public void Rule12g_NonSargable_ColumnSideFunctionInLaterComparison_IsFlagged() + { + Assert.Equal("Function call (DATEPART) on column", PlanAnalyzer.DetectNonSargablePattern( + "[db].[dbo].[T].[A] as [t].[A]=(1) " + + "AND datepart(year,[db].[dbo].[T].[D] as [t].[D])=(2013)")); + } + + /// + /// SQL Server keeps the parentheses of a nested OR, so the predicate splits at every AND/OR + /// depth, not only the top level. Splitting at the top level alone would leave the group as + /// one piece, and its first operator would again put [t].[C] on the upper()'s side. + /// + [Fact] + public void Rule12g_NonSargable_ParameterSideFunctionInsideParenthesizedOr_NotFlagged() + { + Assert.Null(PlanAnalyzer.DetectNonSargablePattern( + "[db].[dbo].[T].[A] as [t].[A]=(1) " + + "AND ([db].[dbo].[T].[B] as [t].[B]=upper([@p]) OR [db].[dbo].[T].[C] as [t].[C]=(3))")); + } + + /// + /// An "and" inside a string literal is text, not an operator. Splitting on it would cut the + /// replace() away from its own comparison, and a piece with no operator in it is treated as + /// the worst case, which turns a parameter-side function into a false warning. + /// + [Fact] + public void Rule12g_NonSargable_AndInsideStringLiteral_DoesNotSplitTheComparison() + { + Assert.Null(PlanAnalyzer.DetectNonSargablePattern( + "replace([@p],N'Tom and Jerry',N'')=[db].[dbo].[T].[Name] as [t].[Name]")); + } + + /// + /// LIKE is a comparison too. Without it the side check found no operator, fell back to assuming + /// the worst, and reported the upper() on the PATTERN as a function on the column. A pattern + /// built from a parameter is a runtime constant and seeks fine. + /// + [Fact] + public void Rule12g_NonSargable_ParameterSideFunctionInLikePattern_NotFlagged() + { + Assert.Null(PlanAnalyzer.DetectNonSargablePattern( + "[db].[dbo].[T].[Name] as [t].[Name] like upper([@p])")); + } + + /// + /// The mirror image stays flagged: upper() on the COLUMN side of a LIKE forces every row + /// through the function before the pattern can be applied. + /// + [Fact] + public void Rule12g_NonSargable_ColumnSideFunctionBeforeLike_IsFlagged() + { + Assert.Equal("Function call (UPPER) on column", PlanAnalyzer.DetectNonSargablePattern( + "upper([db].[dbo].[T].[Name] as [t].[Name]) like N'ABC%'")); + } + + /// + /// ISNULL was flagged wherever it appeared, so ISNULL(@p, 0) on the parameter side was reported + /// as "wrapping a column" even though it is a runtime constant that seeks fine. It now gets the + /// same side check as every other function, and in a compound predicate that is the same shape + /// #556 reported, with ISNULL in place of CONVERT. + /// + [Fact] + public void Rule12h_NonSargable_ParameterSideIsnull_NotFlagged() + { + Assert.Null(PlanAnalyzer.DetectNonSargablePattern( + "[db].[dbo].[T].[A] as [t].[A]=(52) AND [db].[dbo].[T].[B] as [t].[B]=isnull([@p],(0))")); + } + + /// + /// The optional-parameter pattern, WHERE col = ISNULL(@p, col), still reads as non-SARGable: the + /// column sits inside the ISNULL, so it shares the function's side of the comparison. + /// + [Fact] + public void Rule12h_NonSargable_IsnullWithColumnFallback_IsFlagged() + { + Assert.Equal("ISNULL/COALESCE wrapping column", PlanAnalyzer.DetectNonSargablePattern( + "[db].[dbo].[T].[Name] as [t].[Name]=isnull([@p],[db].[dbo].[T].[Name] as [t].[Name])")); + } + + /// + /// A parameter-side ISNULL used to win the check order and report "ISNULL/COALESCE wrapping + /// column" for a predicate whose real problem is a function on a column somewhere else. The + /// message now names the function that is actually on the column. + /// + [Fact] + public void Rule12h_NonSargable_ParameterSideIsnullDoesNotMisnameTheRealProblem() + { + Assert.Equal("Function call (DATEPART) on column", PlanAnalyzer.DetectNonSargablePattern( + "datepart(year,[db].[dbo].[T].[D] as [t].[D])=(2013) " + + "AND [db].[dbo].[T].[B] as [t].[B]=isnull([@p],(0))")); + } + // --------------------------------------------------------------- // Rule 12: Non-SARGable Predicate — Function Call // --------------------------------------------------------------- diff --git a/tests/PlanViewer.Core.Tests/Plans/non_sargable_compound_predicate_plan.sqlplan b/tests/PlanViewer.Core.Tests/Plans/non_sargable_compound_predicate_plan.sqlplan new file mode 100644 index 0000000..df55474 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/Plans/non_sargable_compound_predicate_plan.sqlplan @@ -0,0 +1,2 @@ + + diff --git a/tests/PlanViewer.Core.Tests/WarningBaseline.txt b/tests/PlanViewer.Core.Tests/WarningBaseline.txt index 3ffa1da..20ff0a4 100644 --- a/tests/PlanViewer.Core.Tests/WarningBaseline.txt +++ b/tests/PlanViewer.Core.Tests/WarningBaseline.txt @@ -196,6 +196,9 @@ Scan With Predicate | Critical | Scan with residual predicate — SQL Server is ### multi_index_update_plan.sqlplan Expensive Operator | Critical | Clustered Index Update took 1ms (100.0% of statement elapsed) but no specific rule identified a fix. Worth investigating: is the row volume necessary? Are upstream estimates driving this operator harder than it should be? +### non_sargable_compound_predicate_plan.sqlplan +Scan With Predicate | Critical | Scan with residual predicate — SQL Server is reading every row and filtering after the fact. This scan is 100% of the plan cost. Check that you have appropriate indexes.\nPredicate: [Repro].[dbo].[T].[A] as [t].[A]=CONVERT_IMPLICIT(int,[@1],0) AND [Repro].[dbo].[T].[B] as [t].[B]=CONVERT(tinyint,[@2],0) + ### non_sargable_function_plan.sqlplan Wait: LATCH_EX | Info | LATCH_EX Observed 22 ms across 121 waits. Wait: SOS_SCHEDULER_YIELD | Info | SOS_SCHEDULER_YIELD Observed 3 ms across 1,387 waits.