From 893920e78fe014a1109e0cd3f2e85b2bd8458abc Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 15 Apr 2026 23:50:23 -0400 Subject: [PATCH 1/2] Add benefit scoring for parallel skew and key lookup NL join - a17: Parallel Skew (Rule 8) now gets benefit scoring using operator's own elapsed time as % of statement elapsed, same as other operator-time rules - a4: Key Lookup benefit now includes the parent Nested Loops join when the NL only exists to drive the lookup (inner=lookup, outer=simple seek/scan with no children). Handles both actual and estimated plans. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/PlanViewer.Core/Services/BenefitScorer.cs | 68 ++++++++++++++++++- 1 file changed, 65 insertions(+), 3 deletions(-) diff --git a/src/PlanViewer.Core/Services/BenefitScorer.cs b/src/PlanViewer.Core/Services/BenefitScorer.cs index 7fa24a0..a6ef6c0 100644 --- a/src/PlanViewer.Core/Services/BenefitScorer.cs +++ b/src/PlanViewer.Core/Services/BenefitScorer.cs @@ -18,8 +18,7 @@ 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 @@ -150,16 +149,24 @@ 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); } + else if (warning.WarningType == "Parallel Skew") // Rule 8 + { + ScoreByOperatorTime(warning, node, stmt); + } else if (warning.WarningType == "Row Estimate Mismatch") // Rule 5 { ScoreEstimateMismatchWarning(warning, node, stmt); } // Rules that stay null: Scalar UDF (Rule 6, informational reference), - // Parallel Skew (Rule 8), Data Type Mismatch (Rule 13), + // 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 +289,61 @@ private static void ScoreByOperatorTime(PlanWarning warning, PlanNode node, Plan } } + /// + /// 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). + /// + 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); + } + } + /// /// Rule 5: Row Estimate Mismatch — benefit is the harmed operator's time. /// If the mismatch caused a spill, benefit = spilling operator time. From 891f3dcfc7cf89da611747b4384387c71b9d8011 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Thu, 16 Apr 2026 06:44:13 -0400 Subject: [PATCH 2/2] =?UTF-8?q?Remove=20parallel=20skew=20scoring=20?= =?UTF-8?q?=E2=80=94=20will=20be=20integrated=20per-operator=20later?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Joe changed his mind on a17: parallel skew should be a property of each operator (consolidated with other improvements) rather than a standalone benefit score. Reverting the Parallel Skew dispatch; keeping the key lookup NL join scoring (a4). Co-Authored-By: Claude Opus 4.6 (1M context) --- src/PlanViewer.Core/Services/BenefitScorer.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/PlanViewer.Core/Services/BenefitScorer.cs b/src/PlanViewer.Core/Services/BenefitScorer.cs index a6ef6c0..64fd40a 100644 --- a/src/PlanViewer.Core/Services/BenefitScorer.cs +++ b/src/PlanViewer.Core/Services/BenefitScorer.cs @@ -157,15 +157,12 @@ private static void ScoreNodeWarnings(PlanNode node, PlanStatement stmt) { ScoreByOperatorTime(warning, node, stmt); } - else if (warning.WarningType == "Parallel Skew") // Rule 8 - { - ScoreByOperatorTime(warning, node, stmt); - } else if (warning.WarningType == "Row Estimate Mismatch") // Rule 5 { ScoreEstimateMismatchWarning(warning, node, stmt); } // Rules that stay null: Scalar UDF (Rule 6, informational reference), + // 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),