Plan analysis: skip Row Estimate Mismatch on unexecuted operators, skip engine functions in the TVF rule (#4522, #4525)

Darling/Darling.Tests/PlanSync4522Tests.cs

@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System.Linq;
+using PerformanceMonitor.PlanAnalysis;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #4522 (PerformanceStudio#577) — Rule 5 (Row Estimate Mismatch) checked
+/// <c>node.HasActualStats &amp;&amp; node.EstimateRows &gt; 0</c> without also requiring
+/// <c>node.ActualExecutions &gt; 0</c>. An operator in a branch that never ran (an outer join's
+/// inner side, a conditional CASE branch, an OR branch of a concatenation) returns 0 rows
+/// because it never executed, not because the estimate was wrong. The rule now skips operators
+/// with zero executions, the same way rules 11, 12 and 29 already do, and the now-unreachable
+/// <c>ActualExecutions &gt; 0 ? ActualExecutions : 1</c> fallback is removed.
+///
+/// <para>The repro XML is a synthetic actual plan (<c>dbo.t</c>) with two Sort operators under a
+/// Concatenation: one ran zero times (a branch that never executed), one ran once and returned
+/// nothing despite a large estimate.</para>
+/// </summary>
+public sealed class PlanSync4522Tests
+{
+    private const string ReproXmlTemplate = """
+        <ShowPlanXML xmlns="http://schemas.microsoft.com/sqlserver/2004/07/showplan" Version="1.564" Build="16.0.4215.2"><BatchSequence><Batch><Statements>
+        <StmtSimple StatementText="SELECT a FROM dbo.t" StatementId="1" StatementCompId="1" StatementType="SELECT">
+          <QueryPlan CachedPlanSize="16" CompileTime="1" CompileCPU="1" CompileMemory="104">
+            <RelOp NodeId="0" PhysicalOp="Concatenation" LogicalOp="Concatenation" EstimateRows="2000" EstimateIO="0" EstimateCPU="0" AvgRowSize="9" EstimatedTotalSubtreeCost="1" TableCardinality="0" Parallel="0" EstimateRebinds="0" EstimateRewinds="0" EstimatedExecutionMode="Row">
+              <OutputList/>
+              <RunTimeInformation><RunTimeCountersPerThread Thread="0" ActualRows="0" ActualExecutions="1" ActualEndOfScans="1" ActualExecutionMode="Row"/></RunTimeInformation>
+              <Concat>
+                <RelOp NodeId="1" PhysicalOp="Sort" LogicalOp="Sort" EstimateRows="1000" EstimateIO="0" EstimateCPU="0" AvgRowSize="9" EstimatedTotalSubtreeCost="1" TableCardinality="0" Parallel="0" EstimateRebinds="0" EstimateRewinds="0" EstimatedExecutionMode="Row">
+                  <OutputList/>
+                  <RunTimeInformation><RunTimeCountersPerThread Thread="0" ActualRows="0" ActualExecutions="{{FIRST_EXECUTIONS}}" ActualEndOfScans="1" ActualExecutionMode="Row"/></RunTimeInformation>
+                  <Sort Distinct="0"><OrderBy/><DefinedValues/></Sort>
+                </RelOp>
+                <RelOp NodeId="2" PhysicalOp="Sort" LogicalOp="Sort" EstimateRows="1000" EstimateIO="0" EstimateCPU="0" AvgRowSize="9" EstimatedTotalSubtreeCost="1" TableCardinality="0" Parallel="0" EstimateRebinds="0" EstimateRewinds="0" EstimatedExecutionMode="Row">
+                  <OutputList/>
+                  <RunTimeInformation><RunTimeCountersPerThread Thread="0" ActualRows="0" ActualExecutions="{{SECOND_EXECUTIONS}}" ActualEndOfScans="1" ActualExecutionMode="Row"/></RunTimeInformation>
+                  <Sort Distinct="0"><OrderBy/><DefinedValues/></Sort>
+                </RelOp>
+              </Concat>
+            </RelOp>
+          </QueryPlan>
+        </StmtSimple>
+        </Statements></Batch></BatchSequence></ShowPlanXML>
+        """;
+
+    private static ParsedPlan ParseAndAnalyze(long firstExecutions, long secondExecutions)
+    {
+        var xml = ReproXmlTemplate
+            .Replace("{{FIRST_EXECUTIONS}}", firstExecutions.ToString())
+            .Replace("{{SECOND_EXECUTIONS}}", secondExecutions.ToString());
+        var plan = ShowPlanParser.Parse(xml);
+        PlanAnalyzer.Analyze(plan);
+        return plan;
+    }
+
+    private static bool HasRowEstimateMismatch(PlanNode node) =>
+        node.Warnings.Any(w => w.WarningType == "Row Estimate Mismatch");
+
+    private static PlanNode FindSort(ParsedPlan plan, int nodeId)
+    {
+        var root = plan.Batches.SelectMany(b => b.Statements).Single().RootNode!;
+        return Flatten(root).Single(n => n.NodeId == nodeId);
+    }
+
+    private static System.Collections.Generic.IEnumerable<PlanNode> Flatten(PlanNode node)
+    {
+        yield return node;
+        foreach (var child in node.Children)
+            foreach (var descendant in Flatten(child))
+                yield return descendant;
+    }
+
+    [Fact]
+    public void Rule05_OperatorThatNeverExecuted_IsNotAnEstimateMismatch()
+    {
+        // Node 1 never executed (ActualExecutions=0) despite an estimate of 1000 rows.
+        var plan = ParseAndAnalyze(firstExecutions: 0, secondExecutions: 1);
+        var neverRan = FindSort(plan, nodeId: 1);
+
+        Assert.False(HasRowEstimateMismatch(neverRan));
+    }
+
+    [Fact]
+    public void Rule05_OperatorThatExecutedAndReturnedNothing_StillWarns()
+    {
+        // Node 2 ran once (ActualExecutions=1) and still returned 0 rows against an estimate
+        // of 1000 — a real mismatch, not a branch that never ran.
+        var plan = ParseAndAnalyze(firstExecutions: 0, secondExecutions: 1);
+        var ranButEmpty = FindSort(plan, nodeId: 2);
+
+        Assert.True(HasRowEstimateMismatch(ranButEmpty));
+    }
+}

Darling/Darling.Tests/PlanSync4525Tests.cs

@@ -0,0 +1,111 @@
+/*
+ * Copyright (c) 2026 Erik Darling, Darling Data LLC
+ *
+ * This file is part of the SQL Server Performance Monitor.
+ *
+ * Licensed under the MIT License. See LICENSE file in the project root for full license information.
+ */
+
+using System.Linq;
+using PerformanceMonitor.PlanAnalysis;
+using Xunit;
+
+namespace Darling.Tests;
+
+/// <summary>
+/// #4525 (PerformanceStudio#584) — Rule 23 (Table-Valued Function) fired on every
+/// <c>LogicalOp="Table-valued function"</c> operator, including the engine's own functions:
+/// <c>STRING_SPLIT</c>, <c>OPENJSON</c>, <c>GENERATE_SERIES</c>, and every DMV/DMF. Those run as
+/// the same operator, but their <c>Object</c> element names no database and no schema — a
+/// function the user wrote always has both. <see cref="PlanNode.SchemaName"/> now carries the
+/// schema next to <see cref="PlanNode.DatabaseName"/> (both set from the parsed <c>Object</c>
+/// element), and rule 23 skips an operator with neither.
+///
+/// <para>The repro XML is a synthetic actual plan, one variant with no <c>Object</c> element (as
+/// the engine's own functions appear) and one with a <c>dbo</c>/<c>[db]</c> user function.</para>
+/// </summary>
+public sealed class PlanSync4525Tests
+{
+    private const string EngineFunctionXml = """
+        <ShowPlanXML xmlns="http://schemas.microsoft.com/sqlserver/2004/07/showplan" Version="1.564" Build="16.0.4215.2"><BatchSequence><Batch><Statements>
+        <StmtSimple StatementText="SELECT value FROM STRING_SPLIT('a,b', ',')" StatementId="1" StatementCompId="1" StatementType="SELECT">
+          <QueryPlan CachedPlanSize="16" CompileTime="1" CompileCPU="1" CompileMemory="104">
+            <RelOp NodeId="0" PhysicalOp="Table-valued function" LogicalOp="Table-valued function" EstimateRows="1" EstimateIO="0" EstimateCPU="0" AvgRowSize="9" EstimatedTotalSubtreeCost="1" TableCardinality="0" Parallel="0" EstimateRebinds="0" EstimateRewinds="0" EstimatedExecutionMode="Row">
+              <OutputList/>
+              <TableValuedFunction><DefinedValues/><Object Table="[STRING_SPLIT]"/></TableValuedFunction>
+            </RelOp>
+          </QueryPlan>
+        </StmtSimple>
+        </Statements></Batch></BatchSequence></ShowPlanXML>
+        """;
+
+    private const string UserFunctionXml = """
+        <ShowPlanXML xmlns="http://schemas.microsoft.com/sqlserver/2004/07/showplan" Version="1.564" Build="16.0.4215.2"><BatchSequence><Batch><Statements>
+        <StmtSimple StatementText="SELECT a FROM dbo.MyTvf(1)" StatementId="1" StatementCompId="1" StatementType="SELECT">
+          <QueryPlan CachedPlanSize="16" CompileTime="1" CompileCPU="1" CompileMemory="104">
+            <RelOp NodeId="0" PhysicalOp="Table-valued function" LogicalOp="Table-valued function" EstimateRows="100" EstimateIO="0" EstimateCPU="0" AvgRowSize="9" EstimatedTotalSubtreeCost="1" TableCardinality="0" Parallel="0" EstimateRebinds="0" EstimateRewinds="0" EstimatedExecutionMode="Row">
+              <OutputList/>
+              <TableValuedFunction><DefinedValues/><Object Database="[db]" Schema="[dbo]" Table="[MyTvf]"/></TableValuedFunction>
+            </RelOp>
+          </QueryPlan>
+        </StmtSimple>
+        </Statements></Batch></BatchSequence></ShowPlanXML>
+        """;
+
+    private static ParsedPlan ParseAndAnalyze(string xml)
+    {
+        var plan = ShowPlanParser.Parse(xml);
+        PlanAnalyzer.Analyze(plan);
+        return plan;
+    }
+
+    private static System.Collections.Generic.IEnumerable<PlanNode> Flatten(PlanNode node)
+    {
+        yield return node;
+        foreach (var child in node.Children)
+            foreach (var descendant in Flatten(child))
+                yield return descendant;
+    }
+
+    // RootNode is the synthetic statement wrapper (NodeId == -1); the parsed Table-valued
+    // function operator is its child.
+    private static PlanNode FindTvfNode(ParsedPlan plan)
+    {
+        var root = plan.Batches.SelectMany(b => b.Statements).Single().RootNode!;
+        return Flatten(root).Single(n => n.LogicalOp == "Table-valued function");
+    }
+
+    [Fact]
+    public void Rule23_EngineFunctionWithNoDatabaseAndNoSchema_DoesNotWarn()
+    {
+        var plan = ParseAndAnalyze(EngineFunctionXml);
+        var node = FindTvfNode(plan);
+
+        Assert.Null(node.DatabaseName);
+        Assert.DoesNotContain(node.Warnings, w => w.WarningType == "Table-Valued Function");
+    }
+
+    [Fact]
+    public void Rule23_UserFunctionWithDatabaseAndSchema_StillWarns()
+    {
+        var plan = ParseAndAnalyze(UserFunctionXml);
+        var node = FindTvfNode(plan);
+
+        Assert.Equal("db", node.DatabaseName);
+        Assert.Contains(node.Warnings, w => w.WarningType == "Table-Valued Function");
+    }
+
+    // Compile-only on dev: PlanNode.SchemaName is a new member (#4525 / PS#584); the parser
+    // now sets it next to DatabaseName from the same <Object> element.
+    [Fact]
+    public void Parser_SetsSchemaName_NextToDatabaseName()
+    {
+        var enginePlan = ParseAndAnalyze(EngineFunctionXml);
+        var engineNode = FindTvfNode(enginePlan);
+        Assert.Null(engineNode.SchemaName);
+
+        var userPlan = ParseAndAnalyze(UserFunctionXml);
+        var userNode = FindTvfNode(userPlan);
+        Assert.Equal("dbo", userNode.SchemaName);
+    }
+}

PerformanceMonitor.PlanAnalysis/PlanAnalyzer.cs

@@ -524,7 +524,10 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt)
         // - A parent join may have chosen the wrong strategy
         // - Root nodes with no parent to harm are skipped
         // - Nodes whose only parents are Parallelism/Top/Sort (no spill) are skipped
+        // An operator that never executed returned zero rows because it never ran, so its
+        // zero is no evidence that the estimate was wrong.
         if (node.HasActualStats && node.EstimateRows > 0
+            && node.ActualExecutions > 0
             && !node.Lookup) // Key lookups are point lookups (1 row per execution) — per-execution estimate is misleading
         {
             if (node.ActualRows == 0)
@@ -546,7 +549,7 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt)
             else
             {
                 // Compare per-execution actuals to estimates (SQL Server estimates are per-execution)
-                var executions = node.ActualExecutions > 0 ? node.ActualExecutions : 1;
+                var executions = node.ActualExecutions;
                 var actualPerExec = (double)node.ActualRows / executions;
                 var ratio = actualPerExec / node.EstimateRows;
                 if (ratio >= 10.0 || ratio <= 0.1)
@@ -1010,7 +1013,12 @@ _ when nonSargableReason.StartsWith("Function call", StringComparison.OrdinalIgn
         }
 
         // Rule 23: Table-valued functions
-        if (node.LogicalOp == "Table-valued function")
+        // A function the engine supplies runs as the same operator: STRING_SPLIT, OPENJSON,
+        // GENERATE_SERIES, and every DMV and DMF. Its Object names no database and no schema,
+        // and a function a user wrote always has both. The advice below is about code the
+        // user can rewrite, so the engine's own functions are skipped.
+        var isEngineFunction = string.IsNullOrEmpty(node.DatabaseName) && string.IsNullOrEmpty(node.SchemaName);
+        if (node.LogicalOp == "Table-valued function" && !isEngineFunction)
         {
             var funcName = node.ObjectName ?? node.PhysicalOp;
             node.Warnings.Add(new PlanWarning

PerformanceMonitor.PlanAnalysis/PlanModels.cs

@@ -198,6 +198,7 @@ public class PlanNode
 
     // Detail properties (for tooltip/properties panel)
     public string? DatabaseName { get; set; }
+    public string? SchemaName { get; set; }
     public string? ObjectName { get; set; }
     public string? FullObjectName { get; set; }
     public string? IndexName { get; set; }

PerformanceMonitor.PlanAnalysis/ShowPlanParser.cs

@@ -716,6 +716,7 @@ private static PlanNode ParseRelOp(XElement relOpEl)
                 var index = objEl.Attribute("Index")?.Value?.Replace("[", "").Replace("]", "");
 
                 node.DatabaseName = db;
+                node.SchemaName = schema;
                 node.IndexName = index;
 
                 var shortParts = new List<string>();