From 6329343271333b8e55c9efe5d93fde4340cfc787 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 27 Feb 2026 11:52:30 -0500 Subject: [PATCH] Rules 15-24: Pattern precision, threshold tuning, and message improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 15: Exact OR expansion chain check (Nested Loops → Merge Interval → TopN Sort → Concatenation → Constant Scans) replaces loose ancestor walks Rule 16: Raise nested loops thresholds to 100K warn / 1M critical Rule 17: In actual plans, only warn when Merge Join has logical reads (worktable was actually used); include read count in message Rule 22: StartsWith("@") instead of Contains; updated message re: column statistics and 2019+ deferred compilation Rule 24: Include columnstore scans in Top Above Scan detection Co-Authored-By: Claude Opus 4.6 --- Dashboard/Services/PlanAnalyzer.cs | 93 +++++++++++++++++------------- Lite/Services/PlanAnalyzer.cs | 93 +++++++++++++++++------------- 2 files changed, 106 insertions(+), 80 deletions(-) diff --git a/Dashboard/Services/PlanAnalyzer.cs b/Dashboard/Services/PlanAnalyzer.cs index f49abb35..13f10f3d 100644 --- a/Dashboard/Services/PlanAnalyzer.cs +++ b/Dashboard/Services/PlanAnalyzer.cs @@ -399,16 +399,16 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) } } - // Rule 15: Join OR clause (Concatenation + Constant Scan under join/Merge Interval) - // Best signal: Merge Interval → TopN Sort → Concatenation → Constant Scans - // Also fires under a join ancestor (broader catch) + // Rule 15: Join OR clause + // Pattern: Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation → [Compute Scalar] → 2+ Constant Scans if (node.PhysicalOp == "Concatenation") { var constantScanBranches = node.Children .Count(c => c.PhysicalOp == "Constant Scan" || - c.Children.Any(gc => gc.PhysicalOp == "Constant Scan")); + (c.PhysicalOp == "Compute Scalar" && + c.Children.Any(gc => gc.PhysicalOp == "Constant Scan"))); - if (constantScanBranches >= 2 && (HasMergeIntervalAncestor(node) || HasJoinAncestor(node))) + if (constantScanBranches >= 2 && IsOrExpansionChain(node)) { node.Warnings.Add(new PlanWarning { @@ -426,25 +426,25 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) { var innerChild = node.Children[1]; - if (innerChild.HasActualStats && innerChild.ActualExecutions > 1000) + if (innerChild.HasActualStats && innerChild.ActualExecutions > 100000) { var dop = stmt.DegreeOfParallelism > 0 ? stmt.DegreeOfParallelism : 1; node.Warnings.Add(new PlanWarning { WarningType = "Nested Loops High Executions", Message = $"Nested Loops inner side executed {innerChild.ActualExecutions:N0} times (DOP {dop}). A Hash Join or Merge Join may be more efficient for this row count.", - Severity = innerChild.ActualExecutions > 100000 + Severity = innerChild.ActualExecutions > 1000000 ? PlanWarningSeverity.Critical : PlanWarningSeverity.Warning }); } - else if (!innerChild.HasActualStats && innerChild.EstimateRebinds > 1000) + else if (!innerChild.HasActualStats && innerChild.EstimateRebinds > 100000) { node.Warnings.Add(new PlanWarning { WarningType = "Nested Loops High Executions", Message = $"Nested Loops inner side estimated to execute {innerChild.EstimateRebinds + 1:N0} times. A Hash Join or Merge Join may be more efficient for this row count.", - Severity = innerChild.EstimateRebinds > 100000 + Severity = innerChild.EstimateRebinds > 1000000 ? PlanWarningSeverity.Critical : PlanWarningSeverity.Warning }); @@ -452,24 +452,29 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) } // Rule 17: Many-to-many Merge Join - if (node.ManyToMany && node.PhysicalOp.Contains("Merge", StringComparison.OrdinalIgnoreCase)) + // In actual plans, the Merge Join operator reports logical reads when the worktable is used. + // When ActualLogicalReads is 0, the worktable wasn't hit and the warning is noise. + if (node.ManyToMany && node.PhysicalOp.Contains("Merge", StringComparison.OrdinalIgnoreCase) && + (!node.HasActualStats || node.ActualLogicalReads > 0)) { node.Warnings.Add(new PlanWarning { WarningType = "Many-to-Many Merge Join", - Message = "Many-to-many Merge Join requires a worktable to handle duplicate values. This can be expensive with large numbers of duplicates.", + Message = node.HasActualStats + ? $"Many-to-many Merge Join used a worktable ({node.ActualLogicalReads:N0} logical reads). This can be expensive with large numbers of duplicates." + : "Many-to-many Merge Join requires a worktable to handle duplicate values. This can be expensive with large numbers of duplicates.", Severity = PlanWarningSeverity.Warning }); } // Rule 22: Table variables (Object name starts with @) if (!string.IsNullOrEmpty(node.ObjectName) && - node.ObjectName.Contains("@")) + node.ObjectName.StartsWith("@")) { node.Warnings.Add(new PlanWarning { WarningType = "Table Variable", - Message = "Table variable detected. Table variables have no statistics, so the optimizer always estimates 1 row regardless of actual cardinality. Consider using a temp table (#table) for better estimates.", + Message = "Table variable detected. Table variables lack column statistics and may lead to poor join choices. Consider using a temp table (#table) for better estimates. On SQL Server 2019+, table variable deferred compilation can improve cardinality estimates.", Severity = PlanWarningSeverity.Warning }); } @@ -505,7 +510,7 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) while (scanCandidate.PhysicalOp == "Compute Scalar" && scanCandidate.Children.Count > 0) scanCandidate = scanCandidate.Children[0]; - if (IsRowstoreScan(scanCandidate)) + if (IsScanOperator(scanCandidate)) { var predInfo = !string.IsNullOrEmpty(scanCandidate.Predicate) ? " The scan has a residual predicate, so it may read many rows before the Top is satisfied." @@ -533,6 +538,17 @@ private static bool IsRowstoreScan(PlanNode node) !node.PhysicalOp.Contains("Columnstore", StringComparison.OrdinalIgnoreCase); } + /// + /// Returns true for any scan operator including columnstore. + /// Excludes spools and constant scans. + /// + private static bool IsScanOperator(PlanNode node) + { + return node.PhysicalOp.Contains("Scan", StringComparison.OrdinalIgnoreCase) && + !node.PhysicalOp.Contains("Spool", StringComparison.OrdinalIgnoreCase) && + !node.PhysicalOp.Contains("Constant", StringComparison.OrdinalIgnoreCase); + } + /// /// Detects non-SARGable patterns in scan predicates. /// Returns a description of the issue, or null if the predicate is fine. @@ -613,34 +629,31 @@ private static void DetectMultiReferenceCte(PlanStatement stmt) } /// - /// Checks whether a node has a Merge Interval ancestor (OR expansion pattern). - /// - private static bool HasMergeIntervalAncestor(PlanNode node) - { - var ancestor = node.Parent; - while (ancestor != null) - { - if (ancestor.PhysicalOp == "Merge Interval") - return true; - ancestor = ancestor.Parent; - } - return false; - } - - /// - /// Checks whether a node has any join ancestor. + /// Verifies the OR expansion chain walking up from a Concatenation node: + /// Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation /// - private static bool HasJoinAncestor(PlanNode node) + private static bool IsOrExpansionChain(PlanNode concatenationNode) { - var ancestor = node.Parent; - while (ancestor != null) - { - if (ancestor.LogicalOp != null && - ancestor.LogicalOp.Contains("Join", StringComparison.OrdinalIgnoreCase)) - return true; - ancestor = ancestor.Parent; - } - return false; + // Walk up, skipping Compute Scalar + var parent = concatenationNode.Parent; + while (parent != null && parent.PhysicalOp == "Compute Scalar") + parent = parent.Parent; + + // Expect TopN Sort + if (parent == null || parent.LogicalOp != "TopN Sort") + return false; + + // Walk up to Merge Interval + parent = parent.Parent; + if (parent == null || parent.PhysicalOp != "Merge Interval") + return false; + + // Walk up to Nested Loops + parent = parent.Parent; + if (parent == null || parent.PhysicalOp != "Nested Loops") + return false; + + return true; } /// diff --git a/Lite/Services/PlanAnalyzer.cs b/Lite/Services/PlanAnalyzer.cs index f87f5be3..efa9b234 100644 --- a/Lite/Services/PlanAnalyzer.cs +++ b/Lite/Services/PlanAnalyzer.cs @@ -399,16 +399,16 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) } } - // Rule 15: Join OR clause (Concatenation + Constant Scan under join/Merge Interval) - // Best signal: Merge Interval → TopN Sort → Concatenation → Constant Scans - // Also fires under a join ancestor (broader catch) + // Rule 15: Join OR clause + // Pattern: Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation → [Compute Scalar] → 2+ Constant Scans if (node.PhysicalOp == "Concatenation") { var constantScanBranches = node.Children .Count(c => c.PhysicalOp == "Constant Scan" || - c.Children.Any(gc => gc.PhysicalOp == "Constant Scan")); + (c.PhysicalOp == "Compute Scalar" && + c.Children.Any(gc => gc.PhysicalOp == "Constant Scan"))); - if (constantScanBranches >= 2 && (HasMergeIntervalAncestor(node) || HasJoinAncestor(node))) + if (constantScanBranches >= 2 && IsOrExpansionChain(node)) { node.Warnings.Add(new PlanWarning { @@ -426,25 +426,25 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) { var innerChild = node.Children[1]; - if (innerChild.HasActualStats && innerChild.ActualExecutions > 1000) + if (innerChild.HasActualStats && innerChild.ActualExecutions > 100000) { var dop = stmt.DegreeOfParallelism > 0 ? stmt.DegreeOfParallelism : 1; node.Warnings.Add(new PlanWarning { WarningType = "Nested Loops High Executions", Message = $"Nested Loops inner side executed {innerChild.ActualExecutions:N0} times (DOP {dop}). A Hash Join or Merge Join may be more efficient for this row count.", - Severity = innerChild.ActualExecutions > 100000 + Severity = innerChild.ActualExecutions > 1000000 ? PlanWarningSeverity.Critical : PlanWarningSeverity.Warning }); } - else if (!innerChild.HasActualStats && innerChild.EstimateRebinds > 1000) + else if (!innerChild.HasActualStats && innerChild.EstimateRebinds > 100000) { node.Warnings.Add(new PlanWarning { WarningType = "Nested Loops High Executions", Message = $"Nested Loops inner side estimated to execute {innerChild.EstimateRebinds + 1:N0} times. A Hash Join or Merge Join may be more efficient for this row count.", - Severity = innerChild.EstimateRebinds > 100000 + Severity = innerChild.EstimateRebinds > 1000000 ? PlanWarningSeverity.Critical : PlanWarningSeverity.Warning }); @@ -452,24 +452,29 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) } // Rule 17: Many-to-many Merge Join - if (node.ManyToMany && node.PhysicalOp.Contains("Merge", StringComparison.OrdinalIgnoreCase)) + // In actual plans, the Merge Join operator reports logical reads when the worktable is used. + // When ActualLogicalReads is 0, the worktable wasn't hit and the warning is noise. + if (node.ManyToMany && node.PhysicalOp.Contains("Merge", StringComparison.OrdinalIgnoreCase) && + (!node.HasActualStats || node.ActualLogicalReads > 0)) { node.Warnings.Add(new PlanWarning { WarningType = "Many-to-Many Merge Join", - Message = "Many-to-many Merge Join requires a worktable to handle duplicate values. This can be expensive with large numbers of duplicates.", + Message = node.HasActualStats + ? $"Many-to-many Merge Join used a worktable ({node.ActualLogicalReads:N0} logical reads). This can be expensive with large numbers of duplicates." + : "Many-to-many Merge Join requires a worktable to handle duplicate values. This can be expensive with large numbers of duplicates.", Severity = PlanWarningSeverity.Warning }); } // Rule 22: Table variables (Object name starts with @) if (!string.IsNullOrEmpty(node.ObjectName) && - node.ObjectName.Contains("@")) + node.ObjectName.StartsWith("@")) { node.Warnings.Add(new PlanWarning { WarningType = "Table Variable", - Message = "Table variable detected. Table variables have no statistics, so the optimizer always estimates 1 row regardless of actual cardinality. Consider using a temp table (#table) for better estimates.", + Message = "Table variable detected. Table variables lack column statistics and may lead to poor join choices. Consider using a temp table (#table) for better estimates. On SQL Server 2019+, table variable deferred compilation can improve cardinality estimates.", Severity = PlanWarningSeverity.Warning }); } @@ -505,7 +510,7 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt) while (scanCandidate.PhysicalOp == "Compute Scalar" && scanCandidate.Children.Count > 0) scanCandidate = scanCandidate.Children[0]; - if (IsRowstoreScan(scanCandidate)) + if (IsScanOperator(scanCandidate)) { var predInfo = !string.IsNullOrEmpty(scanCandidate.Predicate) ? " The scan has a residual predicate, so it may read many rows before the Top is satisfied." @@ -533,6 +538,17 @@ private static bool IsRowstoreScan(PlanNode node) !node.PhysicalOp.Contains("Columnstore", StringComparison.OrdinalIgnoreCase); } + /// + /// Returns true for any scan operator including columnstore. + /// Excludes spools and constant scans. + /// + private static bool IsScanOperator(PlanNode node) + { + return node.PhysicalOp.Contains("Scan", StringComparison.OrdinalIgnoreCase) && + !node.PhysicalOp.Contains("Spool", StringComparison.OrdinalIgnoreCase) && + !node.PhysicalOp.Contains("Constant", StringComparison.OrdinalIgnoreCase); + } + /// /// Detects non-SARGable patterns in scan predicates. /// Returns a description of the issue, or null if the predicate is fine. @@ -613,34 +629,31 @@ private static void DetectMultiReferenceCte(PlanStatement stmt) } /// - /// Checks whether a node has a Merge Interval ancestor (OR expansion pattern). - /// - private static bool HasMergeIntervalAncestor(PlanNode node) - { - var ancestor = node.Parent; - while (ancestor != null) - { - if (ancestor.PhysicalOp == "Merge Interval") - return true; - ancestor = ancestor.Parent; - } - return false; - } - - /// - /// Checks whether a node has any join ancestor. + /// Verifies the OR expansion chain walking up from a Concatenation node: + /// Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation /// - private static bool HasJoinAncestor(PlanNode node) + private static bool IsOrExpansionChain(PlanNode concatenationNode) { - var ancestor = node.Parent; - while (ancestor != null) - { - if (ancestor.LogicalOp != null && - ancestor.LogicalOp.Contains("Join", StringComparison.OrdinalIgnoreCase)) - return true; - ancestor = ancestor.Parent; - } - return false; + // Walk up, skipping Compute Scalar + var parent = concatenationNode.Parent; + while (parent != null && parent.PhysicalOp == "Compute Scalar") + parent = parent.Parent; + + // Expect TopN Sort + if (parent == null || parent.LogicalOp != "TopN Sort") + return false; + + // Walk up to Merge Interval + parent = parent.Parent; + if (parent == null || parent.PhysicalOp != "Merge Interval") + return false; + + // Walk up to Nested Loops + parent = parent.Parent; + if (parent == null || parent.PhysicalOp != "Nested Loops") + return false; + + return true; } ///