Merge pull request #180 from erikdarlingdata/fix/issue-178-warning-improvements

Fix false positive warnings and UI improvements (#178)

Author: erikdarlingdata

src/PlanViewer.Core/Services/PlanAnalyzer.cs

@@ -124,7 +124,9 @@ private static void TryOverrideSeverity(PlanWarning warning, AnalyzerConfig cfg)
     private static void AnalyzeStatement(PlanStatement stmt, AnalyzerConfig cfg)
     {
         // Rule 3: Serial plan with reason
-        if (!cfg.IsRuleDisabled(3) && !string.IsNullOrEmpty(stmt.NonParallelPlanReason))
+        // Skip trivial statements (e.g., variable assignments, constant scans) — not worth warning about
+        if (!cfg.IsRuleDisabled(3) && !string.IsNullOrEmpty(stmt.NonParallelPlanReason)
+            && stmt.StatementSubTreeCost >= 0.01)
         {
             var reason = stmt.NonParallelPlanReason switch
             {
@@ -226,15 +228,16 @@ private static void AnalyzeStatement(PlanStatement stmt, AnalyzerConfig cfg)
             stmt.PlanWarnings.Add(new PlanWarning
             {
                 WarningType = "UDF Execution",
-                Message = $"Scalar UDF cost in this statement: {stmt.QueryUdfElapsedTimeMs:N0}ms elapsed, {stmt.QueryUdfCpuTimeMs:N0}ms CPU. Scalar UDFs run once per row and prevent parallelism. Rewrite as an inline table-valued function, or dump results to a #temp table and apply the UDF only to the final result set.",
+                Message = $"Scalar UDF cost in this statement: {stmt.QueryUdfElapsedTimeMs:N0}ms elapsed, {stmt.QueryUdfCpuTimeMs:N0}ms CPU. Scalar UDFs run once per row and prevent parallelism. Options: rewrite as an inline table-valued function, assign the result to a variable if only one row is needed, dump results to a #temp table and apply the UDF to the final result set, or on SQL Server 2019+ check if the UDF is eligible for automatic scalar UDF inlining.",
                 Severity = stmt.QueryUdfElapsedTimeMs >= 1000 ? PlanWarningSeverity.Critical : PlanWarningSeverity.Warning
             });
         }
 
         // Rule 20: Local variables without RECOMPILE
         // Parameters with no CompiledValue are likely local variables — the optimizer
         // cannot sniff their values and uses density-based ("unknown") estimates.
-        if (!cfg.IsRuleDisabled(20) && stmt.Parameters.Count > 0)
+        // Skip trivial statements (simple variable assignments) where estimate quality doesn't matter.
+        if (!cfg.IsRuleDisabled(20) && stmt.Parameters.Count > 0 && stmt.StatementSubTreeCost >= 0.01)
         {
             var unsnifffedParams = stmt.Parameters
                 .Where(p => string.IsNullOrEmpty(p.CompiledValue))
@@ -441,21 +444,42 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt, AnalyzerConfi
     {
         // Rule 1: Filter operators — rows survived the tree just to be discarded
         // Quantify the impact by summing child subtree cost (reads, CPU, time).
-        if (!cfg.IsRuleDisabled(1) && node.PhysicalOp == "Filter" && !string.IsNullOrEmpty(node.Predicate))
+        // Suppress when the filter's child subtree is trivial (low I/O, fast, cheap).
+        if (!cfg.IsRuleDisabled(1) && node.PhysicalOp == "Filter" && !string.IsNullOrEmpty(node.Predicate)
+            && node.Children.Count > 0)
         {
-            var impact = QuantifyFilterImpact(node);
-            var predicate = Truncate(node.Predicate, 200);
-            var message = "Filter operator discarding rows late in the plan.";
-            if (!string.IsNullOrEmpty(impact))
-                message += $"\n{impact}";
-            message += $"\nPredicate: {predicate}";
+            // Gate: skip trivial filters based on actual stats or estimated cost
+            bool isTrivial;
+            if (node.HasActualStats)
+            {
+                long childReads = 0;
+                foreach (var child in node.Children)
+                    childReads += SumSubtreeReads(child);
+                var childElapsed = node.Children.Max(c => c.ActualElapsedMs);
+                isTrivial = childReads < 128 && childElapsed < 10;
+            }
+            else
+            {
+                var childCost = node.Children.Sum(c => c.EstimatedTotalSubtreeCost);
+                isTrivial = childCost < 1.0;
+            }
 
-            node.Warnings.Add(new PlanWarning
+            if (!isTrivial)
             {
-                WarningType = "Filter Operator",
-                Message = message,
-                Severity = PlanWarningSeverity.Warning
-            });
+                var impact = QuantifyFilterImpact(node);
+                var predicate = Truncate(node.Predicate, 200);
+                var message = "Filter operator discarding rows late in the plan.";
+                if (!string.IsNullOrEmpty(impact))
+                    message += $"\n{impact}";
+                message += $"\nPredicate: {predicate}";
+
+                node.Warnings.Add(new PlanWarning
+                {
+                    WarningType = "Filter Operator",
+                    Message = message,
+                    Severity = PlanWarningSeverity.Warning
+                });
+            }
         }
 
         // Rule 2: Eager Index Spools — optimizer building temporary indexes on the fly
@@ -480,7 +504,7 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt, AnalyzerConfi
             node.Warnings.Add(new PlanWarning
             {
                 WarningType = "UDF Execution",
-                Message = $"Scalar UDF executing on this operator ({node.UdfElapsedTimeMs:N0}ms elapsed, {node.UdfCpuTimeMs:N0}ms CPU). Scalar UDFs run once per row and prevent parallelism. Rewrite as an inline table-valued function, or dump the query results to a #temp table first and apply the UDF only to the final result set.",
+                Message = $"Scalar UDF executing on this operator ({node.UdfElapsedTimeMs:N0}ms elapsed, {node.UdfCpuTimeMs:N0}ms CPU). Scalar UDFs run once per row and prevent parallelism. Options: rewrite as an inline table-valued function, assign the result to a variable if only one row is needed, dump results to a #temp table and apply the UDF to the final result set, or on SQL Server 2019+ check if the UDF is eligible for automatic scalar UDF inlining.",
                 Severity = node.UdfElapsedTimeMs >= 1000 ? PlanWarningSeverity.Critical : PlanWarningSeverity.Warning
             });
         }
@@ -541,7 +565,7 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt, AnalyzerConfi
             node.Warnings.Add(new PlanWarning
             {
                 WarningType = "Scalar UDF",
-                Message = $"Scalar {type} UDF: {udf.FunctionName}. Scalar UDFs run once per row and prevent parallelism. Rewrite as an inline table-valued function, or dump results to a #temp table and apply the UDF only to the final result set.",
+                Message = $"Scalar {type} UDF: {udf.FunctionName}. Scalar UDFs run once per row and prevent parallelism. Options: rewrite as an inline table-valued function, assign the result to a variable if only one row is needed, dump results to a #temp table and apply the UDF to the final result set, or on SQL Server 2019+ check if the UDF is eligible for automatic scalar UDF inlining.",
                 Severity = PlanWarningSeverity.Warning
             });
         }
@@ -938,12 +962,17 @@ _ when nonSargableReason.StartsWith("Function call") =>
             node.EstimateRowsWithoutRowGoal > node.EstimateRows)
         {
             var reduction = node.EstimateRowsWithoutRowGoal / node.EstimateRows;
-            node.Warnings.Add(new PlanWarning
+            // Require at least a 2x reduction to be worth mentioning — "1 to 1" or
+            // tiny floating-point differences that display identically are noise
+            if (reduction >= 2.0)
             {
-                WarningType = "Row Goal",
-                Message = $"Row goal active: estimate reduced from {node.EstimateRowsWithoutRowGoal:N0} to {node.EstimateRows:N0} ({reduction:N0}x reduction) due to TOP, EXISTS, IN, or FAST hint. The optimizer chose this plan shape expecting to stop reading early. If the query reads all rows anyway, the plan choice may be suboptimal.",
-                Severity = PlanWarningSeverity.Info
-            });
+                node.Warnings.Add(new PlanWarning
+                {
+                    WarningType = "Row Goal",
+                    Message = $"Row goal active: estimate reduced from {node.EstimateRowsWithoutRowGoal:N0} to {node.EstimateRows:N0} ({reduction:N0}x reduction) due to TOP, EXISTS, IN, or FAST hint. The optimizer chose this plan shape expecting to stop reading early. If the query reads all rows anyway, the plan choice may be suboptimal.",
+                    Severity = PlanWarningSeverity.Info
+                });
+            }
         }
 
         // Rule 28: Row Count Spool — NOT IN with nullable column
@@ -1177,6 +1206,13 @@ private static bool IsOrExpansionChain(PlanNode concatenationNode)
         if (parent == null || parent.PhysicalOp != "Nested Loops")
             return false;
 
+        // If this Nested Loops is inside an Anti/Semi Join, this is a NOT IN/IN
+        // subquery pattern (Merge Interval optimizing range lookups), not an OR expansion
+        var nlParent = parent.Parent;
+        if (nlParent != null && nlParent.LogicalOp != null &&
+            nlParent.LogicalOp.Contains("Semi"))
+            return false;
+
         return true;
     }
 

src/PlanViewer.Web/Layout/MainLayout.razor

@@ -2,7 +2,7 @@
 
 <header>
     <div class="header-content">
-        <img src="darling-data-logo.png" alt="Erik Darling Data" class="header-logo" />
+        <img src="darling-data-logo.jpg" alt="Darling Data" class="header-logo" />
         <span class="header-divider"></span>
         <span class="header-title">Performance Studio</span>
     </div>

src/PlanViewer.Web/Pages/Index.razor

@@ -59,16 +59,24 @@ else
     @if (result.Statements.Count > 1)
     {
         <div class="statement-tabs">
-            @for (int si = 0; si < result.Statements.Count; si++)
+            @foreach (var entry in SortedStatementIndexes)
             {
-                var idx = si;
+                var idx = entry;
                 var isActive = idx == activeStatement;
+                var s = result.Statements[idx];
                 <button class="stmt-tab @(isActive ? "active" : "")" @onclick="() => SelectStatement(idx)">
                     Statement @(idx + 1)
-                    <span class="stmt-tab-cost">@result.Statements[idx].EstimatedCost.ToString("N2")</span>
-                    @if (result.Statements[idx].Warnings.Count > 0)
+                    @if (s.QueryTime != null)
                     {
-                        <span class="stmt-tab-warns">@result.Statements[idx].Warnings.Count</span>
+                        <span class="stmt-tab-time">@FormatMs(s.QueryTime.ElapsedTimeMs)</span>
+                    }
+                    else
+                    {
+                        <span class="stmt-tab-cost">@s.EstimatedCost.ToString("N2")</span>
+                    }
+                    @if (s.Warnings.Count > 0)
+                    {
+                        <span class="stmt-tab-warns">@s.Warnings.Count</span>
                     }
                 </button>
             }
@@ -240,7 +248,17 @@ else
     @if (GetAllWarnings(ActiveStmt!).Count > 0)
     {
         <div class="warnings-strip">
-            <h4>Warnings <span class="warn-count-badge">@GetAllWarnings(ActiveStmt!).Count</span></h4>
+            <h4>Warnings
+                @{
+                    var allWarns = GetAllWarnings(ActiveStmt!);
+                    var critCount = allWarns.Count(w => w.Severity == "Critical");
+                    var warnCount = allWarns.Count(w => w.Severity == "Warning");
+                    var infoCount = allWarns.Count(w => w.Severity == "Info");
+                }
+                @if (critCount > 0) { <span class="warn-count-badge critical">@critCount</span> }
+                @if (warnCount > 0) { <span class="warn-count-badge warning">@warnCount</span> }
+                @if (infoCount > 0) { <span class="warn-count-badge info">@infoCount</span> }
+            </h4>
             <div class="warnings-list">
                 @foreach (var w in GetAllWarnings(ActiveStmt!))
                 {
@@ -1929,6 +1947,19 @@ else
 
     private bool IsRootNode => selectedNode != null && ActiveStmtPlan?.RootNode == selectedNode;
 
+    // Sort statement tabs: actual plans by elapsed time (desc), estimated by cost (desc)
+    private IEnumerable<int> SortedStatementIndexes
+    {
+        get
+        {
+            if (result == null) return Enumerable.Empty<int>();
+            var indexes = Enumerable.Range(0, result.Statements.Count);
+            if (result.Summary.HasActualStats)
+                return indexes.OrderByDescending(i => result.Statements[i].QueryTime?.ElapsedTimeMs ?? 0);
+            return indexes.OrderByDescending(i => result.Statements[i].EstimatedCost);
+        }
+    }
+
     private static string GetOperatorLabel(PlanNode node)
     {
         if (node.PhysicalOp == "Parallelism" && !string.IsNullOrEmpty(node.LogicalOp) && node.LogicalOp != "Parallelism")

src/PlanViewer.Web/wwwroot/css/app.css

@@ -72,7 +72,7 @@ header {
 }
 
 .header-logo {
-    height: 28px;
+    height: 40px;
     width: auto;
 }
 
@@ -317,12 +317,17 @@ textarea::placeholder {
     color: var(--text);
 }
 
-.stmt-tab-cost {
+.stmt-tab-cost, .stmt-tab-time {
     color: var(--text-muted);
     margin-left: 0.25rem;
     font-size: 0.75rem;
 }
 
+.stmt-tab-time {
+    color: var(--text-secondary);
+    font-weight: 500;
+}
+
 .stmt-tab-warns {
     display: inline-block;
     background: var(--warning-color);
@@ -342,6 +347,11 @@ textarea::placeholder {
     margin-bottom: 0.75rem;
 }
 
+/* Wait stats card gets extra width when present */
+.insight-card.waits.has-items {
+    grid-column: span 2;
+}
+
 .insight-card {
     border-radius: 6px;
     border: 1px solid var(--border);
@@ -514,7 +524,7 @@ textarea::placeholder {
 }
 
 .wait-type {
-    min-width: 120px;
+    min-width: 180px;
     font-family: 'Cascadia Code', 'Consolas', monospace;
     font-size: 0.7rem;
 }
@@ -561,13 +571,17 @@ textarea::placeholder {
 }
 
 .warn-count-badge {
-    background: var(--critical);
     color: #fff;
     font-size: 0.65rem;
     padding: 0.05rem 0.4rem;
     border-radius: 8px;
+    background: var(--critical);
 }
 
+.warn-count-badge.critical { background: var(--critical); }
+.warn-count-badge.warning { background: var(--warning-color); }
+.warn-count-badge.info { background: var(--accent); }
+
 .warnings-list {
     padding: 0.5rem 0.75rem;
     max-height: 300px;

src/PlanViewer.Web/wwwroot/darling-data-logo.jpg

@@ -778,4 +778,70 @@ public void NoJoinPredicate_AppearsInTextFormatterOutput()
         Assert.Contains("No Join Predicate", text);
         Assert.Contains("often misleading", text);
     }
+
+    // ---------------------------------------------------------------
+    // Issue #178: Warning improvement verification (test1.sqlplan)
+    // ---------------------------------------------------------------
+
+    [Fact]
+    public void Issue178_5_SerialPlanSuppressedOnTrivialStatement()
+    {
+        // Statement 1 is a trivial variable assignment (cost ~0.000001) — no Serial Plan warning
+        // Uses private test plan from .internal (not committed to git)
+        var plan = PlanTestHelper.LoadFromInternal("test1.sqlplan");
+        if (plan == null) return; // Skip if plan not available
+        var stmt1 = plan.Batches.SelectMany(b => b.Statements).First();
+
+        Assert.True(stmt1.StatementSubTreeCost < 0.01, "Statement 1 should be trivial cost");
+        Assert.DoesNotContain(stmt1.PlanWarnings, w => w.WarningType == "Serial Plan");
+    }
+
+    [Fact]
+    public void Issue178_9_JoinOrNotTriggeredByMergeInterval()
+    {
+        // Statement 8 has a Merge Interval inside a NOT IN anti-semi join — not a genuine OR expansion
+        var plan = PlanTestHelper.LoadFromInternal("test1.sqlplan");
+        if (plan == null) return;
+        var stmt8 = plan.Batches.SelectMany(b => b.Statements).ElementAt(7);
+        var allNodeWarnings = PlanTestHelper.AllNodeWarnings(stmt8);
+
+        Assert.DoesNotContain(allNodeWarnings, w => w.WarningType == "Join OR Clause");
+    }
+
+    [Fact]
+    public void Issue178_12_RowGoal1to1Suppressed()
+    {
+        // Row Goal "1 to 1 (1x reduction)" should not fire — requires >= 2x reduction
+        var plan = PlanTestHelper.LoadFromInternal("test1.sqlplan");
+        if (plan == null) return;
+        var allWarnings = PlanTestHelper.AllWarnings(plan);
+
+        Assert.DoesNotContain(allWarnings, w =>
+            w.WarningType == "Row Goal" && w.Message.Contains("1x reduction"));
+    }
+
+    [Fact]
+    public void Issue178_6_LocalVariableSuppressedOnTrivialStatement()
+    {
+        // Statement 1 is a trivial variable assignment (cost ~0.000001) — no Local Variables warning
+        var plan = PlanTestHelper.LoadFromInternal("test1.sqlplan");
+        if (plan == null) return;
+        var stmt1 = plan.Batches.SelectMany(b => b.Statements).First();
+
+        Assert.True(stmt1.StatementSubTreeCost < 0.01);
+        Assert.DoesNotContain(stmt1.PlanWarnings, w => w.WarningType == "Local Variables");
+    }
+
+    [Fact]
+    public void Issue178_7_FilterSuppressedOnTrivialChildIO()
+    {
+        // Statement 5 has a Filter with 19 reads and 0-1ms child — should be suppressed
+        var plan = PlanTestHelper.LoadFromInternal("test1.sqlplan");
+        if (plan == null) return;
+        var stmt5 = plan.Batches.SelectMany(b => b.Statements).ElementAt(4);
+        var filterWarnings = PlanTestHelper.AllNodeWarnings(stmt5)
+            .Where(w => w.WarningType == "Filter Operator").ToList();
+
+        Assert.Empty(filterWarnings);
+    }
 }

tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs

@@ -80,6 +80,39 @@ public static PlanStatement FirstStatement(ParsedPlan plan)
         return null;
     }
 
+    /// <summary>
+    /// Loads a plan from .internal/examples (private plans not committed to git).
+    /// Returns null if the file doesn't exist so tests can skip gracefully.
+    /// </summary>
+    public static ParsedPlan? LoadFromInternal(string planFileName)
+    {
+        // Walk up from bin/Debug/net8.0 to find the repo root
+        var dir = new DirectoryInfo(AppContext.BaseDirectory);
+        while (dir != null && !Directory.Exists(Path.Combine(dir.FullName, ".internal")))
+            dir = dir.Parent;
+        if (dir == null) return null;
+
+        var path = Path.Combine(dir.FullName, ".internal", "examples", planFileName);
+        if (!File.Exists(path)) return null;
+
+        var xml = File.ReadAllText(path);
+        xml = xml.Replace("encoding=\"utf-16\"", "encoding=\"utf-8\"");
+        var plan = ShowPlanParser.Parse(xml);
+        PlanAnalyzer.Analyze(plan);
+        return plan;
+    }
+
+    /// <summary>
+    /// Gets all node-level warnings for a single statement.
+    /// </summary>
+    public static List<PlanWarning> AllNodeWarnings(PlanStatement stmt)
+    {
+        var warnings = new List<PlanWarning>();
+        if (stmt.RootNode != null)
+            CollectNodeWarnings(stmt.RootNode, warnings);
+        return warnings;
+    }
+
     private static void CollectNodeWarnings(PlanNode node, List<PlanWarning> warnings)
     {
         warnings.AddRange(node.Warnings);