Read only the function's own comparison in the non-SARGable check

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);
+    }
 
+    /// <summary>
+    /// The pattern half of <see cref="DetectNonSargablePredicate"/>: which non-SARGable shape, if
+    /// any, a predicate ScalarString has.
+    ///
+    /// <para>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.</para>
+    /// </summary>
+    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.
+    ///
+    /// <para><b>Only the function's own comparison is read (#556).</b> 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 <c>[t].[A]=[@1] AND [t].[B]=CONVERT(tinyint,[@2],0)</c>
+    /// the CONVERT looked like it shared a side with [t].[B], and so did the dateadd in the
+    /// everyday range <c>[t].[d]&gt;=dateadd(day,(-7),getdate()) AND [t].[d]&lt;getdate()</c>.</para>
     /// </summary>
     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);
     }
 
+    /// <summary>
+    /// The single comparison around <paramref name="position"/>: the text between the nearest
+    /// AND/OR before it and the nearest after it. <paramref name="offset"/> is where that text
+    /// starts in <paramref name="predicate"/>, so positions can be translated into it.
+    ///
+    /// <para>Operators are split on at every depth, not just the top level: a parenthesized group
+    /// like <c>[t].[A]=(1) AND ([t].[B]=f([@p]) OR [t].[C]=(3))</c> 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.</para>
+    /// </summary>
+    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];
+    }
+
     /// <summary>
     /// Verifies the OR expansion chain walking up from a Concatenation node:
     /// Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation

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);
 

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)

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)
+    // ---------------------------------------------------------------
+
+    /// <summary>
+    /// #556: the side check split the WHOLE predicate at its first comparison operator. In
+    /// <c>[t].[A]=CONVERT_IMPLICIT(int,[@1],0) AND [t].[B]=CONVERT(tinyint,[@2],0)</c> 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.
+    /// </summary>
+    [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"));
+    }
+
+    /// <summary>
+    /// 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".
+    /// </summary>
+    [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]<getdate()"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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)"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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))"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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]"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [Fact]
+    public void Rule12g_NonSargable_ParameterSideFunctionInLikePattern_NotFlagged()
+    {
+        Assert.Null(PlanAnalyzer.DetectNonSargablePattern(
+            "[db].[dbo].[T].[Name] as [t].[Name] like upper([@p])"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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%'"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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))"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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])"));
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [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
     // ---------------------------------------------------------------