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.