Merge pull request #237 from erikdarlingdata/dev

Merge dev to main — Joe #215 feedback through b1/b7

Author: erikdarlingdata

src/PlanViewer.Core/Output/HtmlExporter.cs

@@ -300,6 +300,11 @@ private static void WriteRuntimeCard(StringBuilder sb, StatementResult stmt)
         {
             WriteRow(sb, "Elapsed", $"{stmt.QueryTime.ElapsedTimeMs:N0} ms");
             WriteRow(sb, "CPU", $"{stmt.QueryTime.CpuTimeMs:N0} ms");
+            if (stmt.QueryTime.ElapsedTimeMs > 0)
+            {
+                var ratio = (double)stmt.QueryTime.CpuTimeMs / stmt.QueryTime.ElapsedTimeMs;
+                WriteRow(sb, "CPU:Elapsed", ratio.ToString("N2"));
+            }
         }
         if (stmt.DegreeOfParallelism > 0)
             WriteRow(sb, "DOP", stmt.DegreeOfParallelism.ToString());

src/PlanViewer.Core/Output/ResultMapper.cs

@@ -271,7 +271,7 @@ private static OperatorResult MapNode(PlanNode node)
                 Type = w.WarningType,
                 Severity = w.Severity.ToString(),
                 Message = w.Message,
-                Operator = $"{node.PhysicalOp} (Node {node.NodeId})",
+                Operator = FormatOperatorLabel(node),
                 NodeId = node.NodeId,
                 MaxBenefitPercent = w.MaxBenefitPercent,
                 ActionableFix = w.ActionableFix
@@ -316,4 +316,22 @@ private static void CollectNodeWarnings(OperatorResult? node, List<WarningResult
         foreach (var child in node.Children)
             CollectNodeWarnings(child, warnings);
     }
+
+    /// <summary>
+    /// Formats an operator label for the Operator field on warnings.
+    /// Includes object name for data access operators (scans, seeks, lookups)
+    /// where it helps identify which table/index is involved.
+    /// </summary>
+    private static string FormatOperatorLabel(PlanNode node)
+    {
+        if (!string.IsNullOrEmpty(node.ObjectName))
+        {
+            var objRef = !string.IsNullOrEmpty(node.DatabaseName)
+                ? $"{node.DatabaseName}.{node.ObjectName}"
+                : node.ObjectName;
+            return $"{node.PhysicalOp} on {objRef} (Node {node.NodeId})";
+        }
+
+        return $"{node.PhysicalOp} (Node {node.NodeId})";
+    }
 }

src/PlanViewer.Core/Services/BenefitScorer.cs

@@ -18,11 +18,11 @@ public static class BenefitScorer
         "Filter Operator",      // Rule 1
         "Eager Index Spool",    // Rule 2
         "Spill",                // Rule 7
-        "Key Lookup",           // Rule 10
-        "RID Lookup",           // Rule 10 variant
+        // Key Lookup / RID Lookup (Rule 10) handled separately by ScoreKeyLookupWarning
         "Scan With Predicate",  // Rule 11
         "Non-SARGable Predicate", // Rule 12
         "Scan Cardinality Misestimate", // Rule 32
+        "Bare Scan",            // Rule 34
     };
 
     public static void Score(ParsedPlan plan)
@@ -50,44 +50,18 @@ private static void ScoreStatementWarnings(PlanStatement stmt)
         {
             switch (warning.WarningType)
             {
-                case "Ineffective Parallelism":   // Rule 25
-                case "Parallel Wait Bottleneck":  // Rule 31
-                    // These are meta-findings about parallelism efficiency.
-                    // The benefit is the gap between actual and ideal elapsed time.
-                    if (elapsedMs > 0 && stmt.QueryTimeStats != null)
-                    {
-                        var cpu = stmt.QueryTimeStats.CpuTimeMs;
-                        var dop = stmt.DegreeOfParallelism;
-                        if (dop > 1 && cpu > 0)
-                        {
-                            // Ideal elapsed = CPU / DOP. Benefit = (actual - ideal) / actual
-                            var idealElapsed = (double)cpu / dop;
-                            var benefit = Math.Max(0, (elapsedMs - idealElapsed) / elapsedMs * 100);
-                            warning.MaxBenefitPercent = Math.Min(100, Math.Round(benefit, 1));
-                        }
-                    }
-                    break;
-
                 case "Serial Plan": // Rule 3
-                    // Can't know how fast a parallel plan would be, but estimate:
-                    // CPU-bound: benefit up to (1 - 1/maxDOP) * 100%
-                    if (elapsedMs > 0 && stmt.QueryTimeStats != null)
+                    // Per Joe's formula: (cpu * (DOP - 1) / DOP) / elapsed * 100
+                    // Assumes DOP 4 when the plan doesn't tell us. No benefit when cost < 1
+                    // (trivial plans don't gain from parallelism).
+                    if (elapsedMs > 0 && stmt.QueryTimeStats != null && stmt.StatementSubTreeCost >= 1.0)
                     {
                         var cpu = stmt.QueryTimeStats.CpuTimeMs;
-                        // Assume server max DOP — use a conservative 4 if unknown
                         var potentialDop = 4;
-                        if (cpu >= elapsedMs)
-                        {
-                            // CPU-bound: parallelism could help significantly
-                            var benefit = (1.0 - 1.0 / potentialDop) * 100;
-                            warning.MaxBenefitPercent = Math.Round(benefit, 1);
-                        }
-                        else
+                        if (cpu > 0)
                         {
-                            // Not CPU-bound: parallelism helps less
-                            var cpuRatio = (double)cpu / elapsedMs;
-                            var benefit = cpuRatio * (1.0 - 1.0 / potentialDop) * 100;
-                            warning.MaxBenefitPercent = Math.Round(Math.Min(50, benefit), 1);
+                            var benefit = ((double)cpu * (potentialDop - 1) / potentialDop) / elapsedMs * 100;
+                            warning.MaxBenefitPercent = Math.Round(Math.Min(100, benefit), 1);
                         }
                     }
                     break;
@@ -150,6 +124,10 @@ private static void ScoreNodeWarnings(PlanNode node, PlanStatement stmt)
             {
                 ScoreSpillWarning(warning, node, stmt);
             }
+            else if (warning.WarningType is "Key Lookup" or "RID Lookup") // Rule 10
+            {
+                ScoreKeyLookupWarning(warning, node, stmt);
+            }
             else if (OperatorTimeRules.Contains(warning.WarningType))
             {
                 ScoreByOperatorTime(warning, node, stmt);
@@ -159,7 +137,8 @@ private static void ScoreNodeWarnings(PlanNode node, PlanStatement stmt)
                 ScoreEstimateMismatchWarning(warning, node, stmt);
             }
             // Rules that stay null: Scalar UDF (Rule 6, informational reference),
-            // Parallel Skew (Rule 8), Data Type Mismatch (Rule 13),
+            // Parallel Skew (Rule 8 — will be integrated per-operator later),
+            // Data Type Mismatch (Rule 13),
             // Lazy Spool Ineffective (Rule 14), Join OR Clause (Rule 15),
             // Many-to-Many Merge Join (Rule 17), CTE Multiple References (Rule 21),
             // Table Variable (Rule 22), Table-Valued Function (Rule 23),
@@ -282,6 +261,61 @@ private static void ScoreByOperatorTime(PlanWarning warning, PlanNode node, Plan
         }
     }
 
+    /// <summary>
+    /// Rule 10: Key Lookup / RID Lookup — benefit includes the lookup operator's time,
+    /// plus the parent Nested Loops join when the NL only exists to drive the lookup
+    /// (inner child is the lookup, outer child is a seek/scan with no subtree).
+    /// </summary>
+    private static void ScoreKeyLookupWarning(PlanWarning warning, PlanNode node, PlanStatement stmt)
+    {
+        var stmtMs = stmt.QueryTimeStats?.ElapsedTimeMs ?? 0;
+
+        if (node.HasActualStats && stmtMs > 0)
+        {
+            var operatorMs = PlanAnalyzer.GetOperatorOwnElapsedMs(node);
+
+            // Check if the parent NL join is purely a lookup driver:
+            // - Parent is Nested Loops
+            // - Has exactly 2 children
+            // - This node (the lookup) is the inner child (index 1)
+            // - The outer child (index 0) is a simple seek/scan with no children
+            var parent = node.Parent;
+            if (parent != null
+                && parent.PhysicalOp == "Nested Loops"
+                && parent.Children.Count == 2
+                && parent.Children[1] == node
+                && parent.Children[0].Children.Count == 0)
+            {
+                operatorMs += PlanAnalyzer.GetOperatorOwnElapsedMs(parent);
+            }
+
+            if (operatorMs > 0)
+            {
+                var benefit = (double)operatorMs / stmtMs * 100;
+                warning.MaxBenefitPercent = Math.Round(Math.Min(100, benefit), 1);
+            }
+            else
+            {
+                warning.MaxBenefitPercent = 0;
+            }
+        }
+        else if (!node.HasActualStats && stmt.StatementSubTreeCost > 0)
+        {
+            var benefit = (double)node.CostPercent;
+            // Same parent-NL logic for estimated plans
+            var parent = node.Parent;
+            if (parent != null
+                && parent.PhysicalOp == "Nested Loops"
+                && parent.Children.Count == 2
+                && parent.Children[1] == node
+                && parent.Children[0].Children.Count == 0)
+            {
+                benefit += parent.CostPercent;
+            }
+            warning.MaxBenefitPercent = Math.Round(Math.Min(100, benefit), 1);
+        }
+    }
+
     /// <summary>
     /// Rule 5: Row Estimate Mismatch — benefit is the harmed operator's time.
     /// If the mismatch caused a spill, benefit = spilling operator time.