From 62b1fd3c2a28661b9ff50cd8f1860e3cd34ded0c Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Mon, 9 Mar 2026 13:20:33 -0400 Subject: [PATCH] UX polish: advice output, warnings, parallelism efficiency, and node rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix parallelism efficiency formula: use speedup ratio (CPU/elapsed - 1)/(DOP - 1) instead of capacity utilization. CPU ≈ elapsed at DOP 4 now correctly shows ~0%. - Sort operator warnings: Critical before Warning before Info - CREATE INDEX text in warning blocks now uses green CodeBrush - Fix advice indentation: reset inSubSection on blank lines - Estimated cost: reduce precision from F4 to N2 - Disable cost % color highlighting in actual plans (CPU/duration colors suffice) - All node font sizes normalized to 10 (was 9 for rows, object name) - Add rebinds/rewinds section to node hover tooltip - Soften estimate harm messages: "may have caused the optimizer to make poor choices" - Skip Rule 14 (Lazy Spool Ineffective) for Lazy Index Spools per sql.kiwi research - Downgrade NoJoinPredicate from Critical to Warning with honest messaging Co-Authored-By: Claude Opus 4.6 --- .../Controls/PlanViewerControl.axaml.cs | 30 ++++++++++++++----- .../Services/AdviceContentBuilder.cs | 10 +++++++ src/PlanViewer.Core/Output/TextFormatter.cs | 11 ++++--- src/PlanViewer.Core/Services/PlanAnalyzer.cs | 13 ++++---- .../Services/ShowPlanParser.cs | 4 +-- .../PlanAnalyzerTests.cs | 9 +++--- 6 files changed, 54 insertions(+), 23 deletions(-) diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs index 5bbc4fa..e96024d 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs @@ -442,9 +442,9 @@ private Border CreateNodeVisual(PlanNode node, int totalWarningCount = -1) HorizontalAlignment = HorizontalAlignment.Center }); - // Cost percentage - IBrush costColor = node.CostPercent >= 50 ? OrangeRedBrush - : node.CostPercent >= 25 ? OrangeBrush + // Cost percentage — only highlight in estimated plans; actual plans use duration/CPU colors + IBrush costColor = !node.HasActualStats && node.CostPercent >= 50 ? OrangeRedBrush + : !node.HasActualStats && node.CostPercent >= 25 ? OrangeBrush : fgBrush; stack.Children.Add(new TextBlock @@ -499,7 +499,7 @@ private Border CreateNodeVisual(PlanNode node, int totalWarningCount = -1) stack.Children.Add(new TextBlock { Text = $"{node.ActualRows:N0} of {estRows:N0}{accuracy}", - FontSize = 9, + FontSize = 10, Foreground = rowBrush, TextAlignment = TextAlignment.Center, HorizontalAlignment = HorizontalAlignment.Center, @@ -514,7 +514,7 @@ private Border CreateNodeVisual(PlanNode node, int totalWarningCount = -1) var objBlock = new TextBlock { Text = node.FullObjectName ?? node.ObjectName, - FontSize = 9, + FontSize = 10, Foreground = fgBrush, TextAlignment = TextAlignment.Center, TextWrapping = TextWrapping.Wrap, @@ -1778,6 +1778,17 @@ private object BuildNodeTooltipContent(PlanNode node, List? allWarn AddTooltipRow(stack, "Actual Executions", $"{node.ActualExecutions:N0}"); } + // Rebinds/Rewinds (spools and other operators with rebind/rewind data) + if (node.EstimateRebinds > 0 || node.EstimateRewinds > 0 + || node.ActualRebinds > 0 || node.ActualRewinds > 0) + { + AddTooltipSection(stack, "Rebinds / Rewinds"); + if (node.EstimateRebinds > 0) AddTooltipRow(stack, "Est. Rebinds", $"{node.EstimateRebinds:N1}"); + if (node.EstimateRewinds > 0) AddTooltipRow(stack, "Est. Rewinds", $"{node.EstimateRewinds:N1}"); + if (node.ActualRebinds > 0) AddTooltipRow(stack, "Actual Rebinds", $"{node.ActualRebinds:N0}"); + if (node.ActualRewinds > 0) AddTooltipRow(stack, "Actual Rewinds", $"{node.ActualRewinds:N0}"); + } + // I/O and CPU estimates if (node.EstimateIO > 0 || node.EstimateCPU > 0 || node.EstimatedRowSize > 0) { @@ -2491,10 +2502,13 @@ static string EfficiencyColor(double pct) => pct >= 80 ? "#E4E6EB" string? dopColor = null; if (statement.QueryTimeStats != null && statement.QueryTimeStats.ElapsedTimeMs > 0 && - statement.QueryTimeStats.CpuTimeMs > 0) + statement.QueryTimeStats.CpuTimeMs > 0 && + statement.DegreeOfParallelism > 1) { - var idealCpu = statement.QueryTimeStats.ElapsedTimeMs * statement.DegreeOfParallelism; - var efficiency = Math.Min(100.0, statement.QueryTimeStats.CpuTimeMs * 100.0 / idealCpu); + // Speedup ratio: CPU/elapsed = 1.0 means serial, = DOP means perfect parallelism + var speedup = (double)statement.QueryTimeStats.CpuTimeMs / statement.QueryTimeStats.ElapsedTimeMs; + var efficiency = Math.Min(100.0, (speedup - 1.0) / (statement.DegreeOfParallelism - 1.0) * 100.0); + efficiency = Math.Max(0.0, efficiency); dopText += $" ({efficiency:N0}% efficient)"; dopColor = EfficiencyColor(efficiency); } diff --git a/src/PlanViewer.App/Services/AdviceContentBuilder.cs b/src/PlanViewer.App/Services/AdviceContentBuilder.cs index 338080c..d59b7f4 100644 --- a/src/PlanViewer.App/Services/AdviceContentBuilder.cs +++ b/src/PlanViewer.App/Services/AdviceContentBuilder.cs @@ -74,6 +74,7 @@ public static StackPanel Build(string content) panel.Children.Add(new Border { Height = 6 }); inCodeBlock = false; isStatementText = false; + inSubSection = false; continue; } @@ -422,6 +423,15 @@ private static Border CreateWarningBlock(string line, SolidColorBrush severityBr tb.Inlines.Add(new Run(part[2..]) { Foreground = ValueBrush }); } + else if (part.StartsWith("CREATE ", StringComparison.OrdinalIgnoreCase) + || part.StartsWith("ON ", StringComparison.OrdinalIgnoreCase) + || part.StartsWith("INCLUDE ", StringComparison.OrdinalIgnoreCase) + || part.StartsWith("WHERE ", StringComparison.OrdinalIgnoreCase)) + { + // SQL DDL lines (CREATE INDEX, ON, INCLUDE, WHERE) + tb.Inlines.Add(new Run("\n" + part) + { Foreground = CodeBrush }); + } else { // Other detail lines diff --git a/src/PlanViewer.Core/Output/TextFormatter.cs b/src/PlanViewer.Core/Output/TextFormatter.cs index 80001d1..eb7f99e 100644 --- a/src/PlanViewer.Core/Output/TextFormatter.cs +++ b/src/PlanViewer.Core/Output/TextFormatter.cs @@ -42,15 +42,16 @@ public static void WriteText(AnalysisResult result, TextWriter writer) writer.WriteLine($"=== Statement {i + 1}: ==="); writer.WriteLine(stmt.StatementText); writer.WriteLine(); - writer.WriteLine($"Estimated cost: {stmt.EstimatedCost:F4}"); + writer.WriteLine($"Estimated cost: {stmt.EstimatedCost:N2}"); if (stmt.DegreeOfParallelism > 0) { var dopLine = $"DOP: {stmt.DegreeOfParallelism}"; - if (stmt.QueryTime != null && stmt.QueryTime.ElapsedTimeMs > 0 && stmt.QueryTime.CpuTimeMs > 0) + if (stmt.QueryTime != null && stmt.QueryTime.ElapsedTimeMs > 0 + && stmt.QueryTime.CpuTimeMs > 0 && stmt.DegreeOfParallelism > 1) { - var idealCpu = stmt.QueryTime.ElapsedTimeMs * stmt.DegreeOfParallelism; - var efficiency = Math.Min(100.0, stmt.QueryTime.CpuTimeMs * 100.0 / idealCpu); + var speedup = (double)stmt.QueryTime.CpuTimeMs / stmt.QueryTime.ElapsedTimeMs; + var efficiency = Math.Clamp((speedup - 1.0) / (stmt.DegreeOfParallelism - 1.0) * 100.0, 0, 100); dopLine += $" ({efficiency:N0}% efficient)"; } writer.WriteLine(dopLine); @@ -294,8 +295,10 @@ private static void WriteGroupedOperatorWarnings(List warnings, T } // Group entries that share the same severity, type, and explanation + // Sort criticals before warnings before info var grouped = entries .GroupBy(e => (e.Severity, e.Explanation ?? "")) + .OrderBy(g => g.Key.Item1 switch { "Critical" => 0, "Warning" => 1, _ => 2 }) .ToList(); foreach (var group in grouped) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index 26ad250..75ae9b2 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -672,7 +672,10 @@ _ when nonSargableReason.StartsWith("Function call") => // Rule 14: Lazy Table Spool unfavorable rebind/rewind ratio // Rebinds = cache misses (child re-executes), rewinds = cache hits (reuse cached result) - if (!cfg.IsRuleDisabled(14) && node.LogicalOp == "Lazy Spool") + // Exclude Lazy Index Spools: they cache by correlated parameter value (like a hash table) + // so rebind/rewind counts are unreliable. See https://www.sql.kiwi/2025/02/lazy-index-spool/ + if (!cfg.IsRuleDisabled(14) && node.LogicalOp == "Lazy Spool" + && !node.PhysicalOp.Contains("Index", StringComparison.OrdinalIgnoreCase)) { var rebinds = node.HasActualStats ? (double)node.ActualRebinds : node.EstimateRebinds; var rewinds = node.HasActualStats ? (double)node.ActualRewinds : node.EstimateRewinds; @@ -1327,8 +1330,8 @@ private static long SumSubtreeReads(PlanNode node) if (node.LogicalOp.Contains("Join", StringComparison.OrdinalIgnoreCase) && !node.IsAdaptive) { return ratio >= 10.0 - ? "The underestimate may have caused the optimizer to choose a suboptimal join strategy." - : "The overestimate may have caused the optimizer to choose a suboptimal join strategy."; + ? "The underestimate may have caused the optimizer to make poor choices." + : "The overestimate may have caused the optimizer to make poor choices."; } // Walk up to check if a parent was harmed by this bad estimate @@ -1355,8 +1358,8 @@ private static long SumSubtreeReads(PlanNode node) return null; // Adaptive join self-corrects — no harm return ratio >= 10.0 - ? $"The underestimate may have caused the optimizer to choose {ancestor.PhysicalOp} when a different join type would be more efficient." - : $"The overestimate may have caused the optimizer to choose {ancestor.PhysicalOp} when a different join type would be more efficient."; + ? $"The underestimate may have caused the optimizer to make poor choices." + : $"The overestimate may have caused the optimizer to make poor choices."; } // Parent Sort/Hash that spilled — downstream bad estimate caused the spill diff --git a/src/PlanViewer.Core/Services/ShowPlanParser.cs b/src/PlanViewer.Core/Services/ShowPlanParser.cs index baf847b..2d3037f 100644 --- a/src/PlanViewer.Core/Services/ShowPlanParser.cs +++ b/src/PlanViewer.Core/Services/ShowPlanParser.cs @@ -1475,8 +1475,8 @@ private static List ParseWarningsFromElement(XElement warningsEl) result.Add(new PlanWarning { WarningType = "No Join Predicate", - Message = "This join has no join predicate (possible cross join)", - Severity = PlanWarningSeverity.Critical + Message = "This join triggered a no join predicate warning, which is worth checking on, but is often misleading. The optimizer may have removed a redundant predicate after simplification.", + Severity = PlanWarningSeverity.Warning }); } diff --git a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs index 60ae56e..5ad756f 100644 --- a/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs +++ b/tests/PlanViewer.Core.Tests/PlanAnalyzerTests.cs @@ -321,14 +321,15 @@ public void Rule13_DataTypeMismatch_DetectedInMismatchedPlan() // --------------------------------------------------------------- [Fact] - public void Rule14_LazySpoolIneffective_DetectsUnfavorableRatio() + public void Rule14_LazySpoolIneffective_SkipsLazyIndexSpools() { + // Lazy Index Spools cache by correlated parameter value (like a hash table) + // so rebind/rewind counts are unreliable — Rule 14 should not fire. + // See https://www.sql.kiwi/2025/02/lazy-index-spool/ var plan = PlanTestHelper.LoadAndAnalyze("lazy_spool_plan.sqlplan"); var warnings = PlanTestHelper.WarningsOfType(plan, "Lazy Spool Ineffective"); - Assert.Single(warnings); - Assert.Contains("rebinds", warnings[0].Message); - Assert.Contains("rewinds", warnings[0].Message); + Assert.Empty(warnings); } // ---------------------------------------------------------------