From f8c03bcd5706a47d71960429d4b293a252acb19d Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 6 Apr 2026 23:59:36 -0400 Subject: [PATCH 1/4] Fix false positive warnings from issue #178 feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rule 3 (Serial Plan): Skip trivial statements with cost < 0.01 — a 0ms variable assignment shouldn't warn about MAXDOP 1 - Rule 15 (Join OR Clause): Exclude Merge Interval patterns inside anti/semi joins — NOT IN subqueries produce the same operator chain but aren't OR expansions - Rule 26 (Row Goal): Require >= 2x reduction — "1 to 1 (1x reduction)" and tiny floating-point differences are noise Tests load from .internal/examples (gitignored) and skip gracefully on CI. Closes items 5, 9, 12 from #178. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 26 +++++++++--- .../PlanAnalyzerTests.cs | 41 +++++++++++++++++++ tests/PlanViewer.Core.Tests/PlanTestHelper.cs | 33 +++++++++++++++ 3 files changed, 94 insertions(+), 6 deletions(-) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index f4c829d..642dc50 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/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 { @@ -938,12 +940,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 +1184,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; } diff --git a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs index 33d0689..dfec23e 100644 --- a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs @@ -778,4 +778,45 @@ 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")); + } } diff --git a/tests/PlanViewer.Core.Tests/PlanTestHelper.cs b/tests/PlanViewer.Core.Tests/PlanTestHelper.cs index f14be76..1e5e85a 100644 --- a/tests/PlanViewer.Core.Tests/PlanTestHelper.cs +++ b/tests/PlanViewer.Core.Tests/PlanTestHelper.cs @@ -80,6 +80,39 @@ public static PlanStatement FirstStatement(ParsedPlan plan) return null; } + /// + /// 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. + /// + 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; + } + + /// + /// Gets all node-level warnings for a single statement. + /// + public static List AllNodeWarnings(PlanStatement stmt) + { + var warnings = new List(); + if (stmt.RootNode != null) + CollectNodeWarnings(stmt.RootNode, warnings); + return warnings; + } + private static void CollectNodeWarnings(PlanNode node, List warnings) { warnings.AddRange(node.Warnings); From dc6a043961e272e1400f77c7f04ec165e061f6da Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 7 Apr 2026 00:06:21 -0400 Subject: [PATCH 2/4] Add impact thresholds to Filter and Local Variable warnings (#178) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rule 1 (Filter Operator): Suppress when child subtree is trivial — actual plans: < 128 reads AND < 10ms; estimated plans: cost < 1.0 - Rule 20 (Local Variables): Gate on statement cost >= 0.01 to skip trivial variable assignments where estimate quality doesn't matter Both fixes work correctly for estimated and actual plans, using cost-based fallbacks when runtime stats aren't available. Closes items 6, 7, 11 from #178. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 48 ++++++++++++++----- .../PlanAnalyzerTests.cs | 25 ++++++++++ 2 files changed, 60 insertions(+), 13 deletions(-) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index 642dc50..26a810e 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -236,7 +236,8 @@ private static void AnalyzeStatement(PlanStatement stmt, AnalyzerConfig cfg) // 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)) @@ -443,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 diff --git a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs index dfec23e..d0656aa 100644 --- a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs @@ -819,4 +819,29 @@ public void Issue178_12_RowGoal1to1Suppressed() 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); + } } From 7b5afdd005bda1b38e41b22ba79acfaa32640fa2 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 7 Apr 2026 00:09:07 -0400 Subject: [PATCH 3/4] Improve UDF message and web UI feedback items (#178) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rule 4/6 (Scalar UDF): Expanded remediation options — mention assigning to a variable, SQL Server 2019+ automatic UDF inlining - Warning badge: Split count by severity (critical/warning/info) with color-coded badges instead of a single combined number - Statement tabs: Show elapsed time instead of cost for actual plans - Wait stats: Wider wait type column (180px) and double-width card when populated for better readability Closes items 2, 3 (partial), 4, 10 from #178. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 6 ++--- src/PlanViewer.Web/Pages/Index.razor | 26 +++++++++++++++++--- src/PlanViewer.Web/wwwroot/css/app.css | 20 ++++++++++++--- 3 files changed, 42 insertions(+), 10 deletions(-) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index 26a810e..b66d8fe 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -228,7 +228,7 @@ 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 }); } @@ -504,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 }); } @@ -565,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 }); } diff --git a/src/PlanViewer.Web/Pages/Index.razor b/src/PlanViewer.Web/Pages/Index.razor index a57d061..8f1e79c 100644 --- a/src/PlanViewer.Web/Pages/Index.razor +++ b/src/PlanViewer.Web/Pages/Index.razor @@ -63,12 +63,20 @@ else { var idx = si; var isActive = idx == activeStatement; + var s = result.Statements[idx]; } @@ -240,7 +248,17 @@ else @if (GetAllWarnings(ActiveStmt!).Count > 0) {
-

Warnings @GetAllWarnings(ActiveStmt!).Count

+

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) { @critCount } + @if (warnCount > 0) { @warnCount } + @if (infoCount > 0) { @infoCount } +

@foreach (var w in GetAllWarnings(ActiveStmt!)) { diff --git a/src/PlanViewer.Web/wwwroot/css/app.css b/src/PlanViewer.Web/wwwroot/css/app.css index 7a6f18e..829ab67 100644 --- a/src/PlanViewer.Web/wwwroot/css/app.css +++ b/src/PlanViewer.Web/wwwroot/css/app.css @@ -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; From 66c2b5f553dff693a1ea725fb086d69fa86d4783 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 7 Apr 2026 00:13:36 -0400 Subject: [PATCH 4/4] Sort statement tabs by time/cost, swap to full Darling Data logo (#178) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Statement tabs sorted by elapsed time (actual plans) or cost (estimated plans), slowest first — problem statement is always immediately visible - Replaced small PNG logo with full Darling Data barbell logo from erikdarling.com, bumped height to 40px for legibility - Default active statement is the first tab (sorted, so the slowest) Closes items 1, 3 from #178. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/PlanViewer.Web/Layout/MainLayout.razor | 2 +- src/PlanViewer.Web/Pages/Index.razor | 17 +++++++++++++++-- src/PlanViewer.Web/wwwroot/css/app.css | 2 +- .../wwwroot/darling-data-logo.jpg | Bin 0 -> 102726 bytes 4 files changed, 17 insertions(+), 4 deletions(-) create mode 100644 src/PlanViewer.Web/wwwroot/darling-data-logo.jpg diff --git a/src/PlanViewer.Web/Layout/MainLayout.razor b/src/PlanViewer.Web/Layout/MainLayout.razor index 3981503..2efde8d 100644 --- a/src/PlanViewer.Web/Layout/MainLayout.razor +++ b/src/PlanViewer.Web/Layout/MainLayout.razor @@ -2,7 +2,7 @@
- + Performance Studio
diff --git a/src/PlanViewer.Web/Pages/Index.razor b/src/PlanViewer.Web/Pages/Index.razor index 8f1e79c..da00a74 100644 --- a/src/PlanViewer.Web/Pages/Index.razor +++ b/src/PlanViewer.Web/Pages/Index.razor @@ -59,9 +59,9 @@ else @if (result.Statements.Count > 1) {
- @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];