From 1a6a671a7b7de23cb70e927a86536f012895958f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:14:44 +0100 Subject: [PATCH] Link every warning to the operator it came from (#440) A warning did not record where it came from, so on a large plan there was no way to get from a finding to the thing that produced it. pgfiore's framing was exactly right: "if the plan is huge and the warning origin is murky, it would help to click a warning and be beamed up to a specific step." PlanWarning.OriginNodeIds carries it, and the interesting half of this feature is knowing when to say nothing. Three honest answers, not one: - A key lookup came from exactly one operator. - A table variable warning came from every operator that touched one, which on a real plan is several. That rule already walked the tree and knew precisely which ones; it threw the answer away before emitting. It does not any more. - "High Compile CPU" happened before a single row was read, and SQL Server reports "UDF Execution" at the statement level only. Those have NO operator origin and now say so, so the UI offers no link rather than one that goes somewhere arbitrary. Sending a reader to the wrong operator is worse than sending them nowhere, because they would believe it. Operator warnings are stamped in ONE place, at the end of AnalyzeNode, rather than at the 26 sites that add one - the same reasoning as the provenance stamp in #439 and the ceiling in #438. A rule you have to remember at every construction site is a rule that eventually gets forgotten, and the once it is forgotten the UI quietly drops a link that existed. It only fills what a rule left empty, so a rule that knows better keeps its own answer. The scope call worth reviewing. The pop-up the issue is about shows STATEMENT warnings, and few of those can attribute to an operator - so linking only those would not have served the huge-plan case that motivated the request at all. The warnings that do have origins are the operator ones, and until now the only way to see one was to have already clicked the operator carrying it, which is no help when you do not know which operator to click. So the statement panel also gains an "Operator Warnings" section indexing every warning in the tree, each one a link. Nothing is removed from the per-operator panel; this is an index into it. It is collapsed by default because on a large plan it is the longest section in the panel and expanding it would push the statement's own details off screen, which is the opposite of the problem being solved. The tree walk lives in Core as WarningIndex rather than beside the panel that renders it: it is a walk over Core's own models with nothing UI about it, and there it can be tested without standing up Avalonia. It uses an explicit stack rather than recursion, because a deep plan is precisely the case this feature exists for and #430 was a crash caused by assuming operator trees are shallow. CLI output contract: "origin_node_ids" is additive on every warning, so ExpectedCompactOutputSha256 is rolled - second time, both additive, both deliberate. Verified that origin_node_ids is the ONLY new key rather than assuming it: the full key set on a warning is otherwise unchanged. WarningBaseline.txt does not move. It digests type, severity and message, none of which changed, so no committed plan's verdict is affected. Tested: 314 total, 312 passed, 0 failed. The new tests pin both directions - that across every committed plan no operator warning is left without an origin or points away from its own node, that the table variable warning names the operators that touch one, and that High Compile CPU and UDF Execution claim none. The index is compared against an independent recursive walk rather than a hand-written count, so it cannot drift as fixtures are added. Not verified: the click itself. This box has no reachable display session, so screencapture fails and I could not watch a warning navigate. The app was launched on a plan carrying both kinds of warning and ran clean with no exceptions, and the logic underneath is covered, but someone with a screen should confirm the scroll lands where it should before this is trusted. Co-Authored-By: Claude Opus 5 (1M context) --- .../Controls/PlanViewerControl.Interaction.cs | 43 ++++++ .../Controls/PlanViewerControl.Properties.cs | 78 +++++++++- .../Controls/PlanViewerControl.Rendering.cs | 40 +++++ src/PlanViewer.Core/Models/PlanModels.cs | 15 ++ src/PlanViewer.Core/Output/AnalysisResult.cs | 9 ++ src/PlanViewer.Core/Output/ResultMapper.cs | 6 +- .../Services/PlanAnalyzer.Detection.cs | 12 +- .../Services/PlanAnalyzer.Node.cs | 14 ++ .../Services/PlanAnalyzer.Statement.cs | 11 +- src/PlanViewer.Core/Services/WarningIndex.cs | 41 +++++ .../HistoricalCliContractTests.cs | 14 +- .../WarningOriginTests.cs | 144 ++++++++++++++++++ 12 files changed, 412 insertions(+), 15 deletions(-) create mode 100644 src/PlanViewer.Core/Services/WarningIndex.cs create mode 100644 tests/PlanViewer.Core.Tests/WarningOriginTests.cs diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs index a67609e..7dd13b2 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs @@ -46,6 +46,49 @@ private void SelectNode(Border border, PlanNode node) UpdateMinimapSelection(node); } + /// + /// Selects the operator with and scrolls it into view, so a warning + /// can take you to where it came from (#440). Returns false when the plan has no such operator, + /// which is what keeps a stale or wrong origin from silently scrolling somewhere arbitrary. + /// + private bool TryNavigateToNode(int nodeId) + { + foreach (var child in PlanCanvas.Children) + { + if (child is not Border border || + !_nodeBorderMap.TryGetValue(border, out var node) || + node.NodeId != nodeId) + { + continue; + } + + SelectNode(border, node); + ScrollNodeIntoView(node); + return true; + } + + return false; + } + + /// + /// Centres the operator in the viewport. Node coordinates are unscaled layout positions, so they + /// are multiplied by the zoom level to get canvas pixels; the offset is then clamped, because + /// asking a ScrollViewer for a negative offset on a plan smaller than the viewport just leaves + /// it where it was and looks like the navigation did nothing. + /// + private void ScrollNodeIntoView(PlanNode node) + { + var targetX = node.X * _zoomLevel - (PlanScrollViewer.Bounds.Width / 2); + var targetY = node.Y * _zoomLevel - (PlanScrollViewer.Bounds.Height / 2); + + var maxX = Math.Max(0, PlanScrollViewer.Extent.Width - PlanScrollViewer.Viewport.Width); + var maxY = Math.Max(0, PlanScrollViewer.Extent.Height - PlanScrollViewer.Viewport.Height); + + PlanScrollViewer.Offset = new Vector( + Math.Clamp(targetX, 0, maxX), + Math.Clamp(targetY, 0, maxY)); + } + private ContextMenu BuildNodeContextMenu(PlanNode node) { var menu = new ContextMenu(); diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs index dbd8369..500514c 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Properties.cs @@ -822,13 +822,15 @@ private void ShowPropertiesPanel(PlanNode node) var planWarnHeader = w.MaxBenefitPercent.HasValue ? $"\u26A0 {w.WarningType}{sourceTag}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" : $"\u26A0 {w.WarningType}{sourceTag}{legacyTag}"; - warnPanel.Children.Add(new TextBlock + var planWarnHeaderBlock = new TextBlock { Text = planWarnHeader, FontWeight = FontWeight.SemiBold, FontSize = 11, Foreground = new SolidColorBrush(Color.Parse(warnColor)) - }); + }; + AttachOriginNavigation(planWarnHeaderBlock, planWarnHeader, w.OriginNodeIds); + warnPanel.Children.Add(planWarnHeaderBlock); warnPanel.Children.Add(new TextBlock { Text = w.Message, @@ -875,6 +877,78 @@ private void ShowPropertiesPanel(PlanNode node) PropertiesContent.Children.Add(planWarningsExpander); } + /* === Operator Warnings (#440) === + Every warning hanging off an operator anywhere in this statement, gathered in one + place and each one a link to its operator. + + This section is the point of #440. The reporter's case was "the plan is huge and the + warning origin is murky", and the operator warnings are exactly the ones with a + murky origin - but until now the only way to see one was to already have clicked the + operator it was on, which is no help when you do not know which operator to click. + Nothing is removed from the per-operator panel; this is an index into it. */ + var operatorWarnings = WarningIndex.CollectOperatorWarnings(s.RootNode); + if (operatorWarnings.Count > 0) + { + var operatorWarningsPanel = new StackPanel(); + foreach (var (originNode, w) in operatorWarnings + .OrderByDescending(x => x.Warning.MaxBenefitPercent ?? -1) + .ThenByDescending(x => x.Warning.Severity) + .ThenBy(x => x.Warning.WarningType)) + { + var opWarnColor = w.Severity == PlanWarningSeverity.Critical ? "#E57373" + : w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF"; + var opWarnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) }; + var opBenefit = w.MaxBenefitPercent.HasValue + ? $" \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit" + : ""; + var opHeaderText = + $"\u26A0 {w.WarningType}{WarningSourceTag(w)}{(w.IsLegacy ? " [legacy]" : "")}{opBenefit}"; + var opHeader = new TextBlock + { + Text = opHeaderText, + FontWeight = FontWeight.SemiBold, + FontSize = 11, + Foreground = new SolidColorBrush(Color.Parse(opWarnColor)) + }; + AttachOriginNavigation(opHeader, opHeaderText, w.OriginNodeIds); + opWarnPanel.Children.Add(opHeader); + opWarnPanel.Children.Add(new TextBlock + { + Text = OperatorOriginLabel(originNode), + FontSize = 11, + Foreground = SectionHeaderBrush, + Margin = new Thickness(16, 0, 0, 0) + }); + operatorWarningsPanel.Children.Add(opWarnPanel); + } + + var operatorWarningsExpander = new Expander + { + /* Collapsed by default, unlike Plan Warnings. On a large plan this is the + longest section in the panel, and expanding it by default would push the + statement's own details off screen - the opposite of the problem #440 is + about. */ + IsExpanded = false, + Header = new TextBlock + { + Text = $"Operator Warnings ({operatorWarnings.Count})", + FontWeight = FontWeight.SemiBold, + FontSize = 11, + Foreground = SectionHeaderBrush + }, + Content = operatorWarningsPanel, + Margin = new Thickness(0, 2, 0, 0), + Padding = new Thickness(0), + Foreground = SectionHeaderBrush, + Background = new SolidColorBrush(Color.FromArgb(0x18, 0x4F, 0xA3, 0xFF)), + BorderBrush = PropSeparatorBrush, + BorderThickness = new Thickness(0, 0, 0, 1), + HorizontalAlignment = HorizontalAlignment.Stretch, + HorizontalContentAlignment = HorizontalAlignment.Stretch + }; + PropertiesContent.Children.Add(operatorWarningsExpander); + } + // === Missing Indexes === if (s.MissingIndexes.Count > 0) { diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs index ae4ef22..ca337fe 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Rendering.cs @@ -516,6 +516,46 @@ private static string FormatBytes(double bytes) private static string FormatBenefitPercent(double pct) => pct >= 100 ? $"{pct:N0}" : $"{pct:N1}"; + /// + /// How an operator is named in the aggregated warning index (#440) — enough to recognise it + /// before clicking, matching what the operator's own panel puts in its header. + /// + private static string OperatorOriginLabel(PlanNode node) => + string.IsNullOrEmpty(node.FullObjectName) + ? $"Node {node.NodeId} \u00B7 {node.PhysicalOp}" + : $"Node {node.NodeId} \u00B7 {node.PhysicalOp} on {node.FullObjectName}"; + + /// + /// Turns a warning header into a link to the operator it came from (#440). + /// + /// Only when the warning actually knows. Findings with no operator origin — "High Compile + /// CPU" happened before a row was read — are left as plain text rather than given a link that + /// goes somewhere arbitrary, because a reader would believe it. + /// + /// Where a warning came from several operators, the first is the navigation target and the + /// rest are named in the tooltip, so the count is visible rather than silently dropped. + /// + private void AttachOriginNavigation(TextBlock header, string headerText, List originNodeIds) + { + if (originNodeIds.Count == 0) + return; + + header.Text = headerText + " \u2192"; + header.Cursor = new Cursor(StandardCursorType.Hand); + ToolTip.SetTip(header, originNodeIds.Count == 1 + ? $"Go to operator (Node {originNodeIds[0]})" + : $"Go to Node {originNodeIds[0]} — also from {string.Join(", ", originNodeIds.Skip(1).Select(id => "Node " + id))}"); + + header.PointerPressed += (_, e) => + { + if (!e.GetCurrentPoint(header).Properties.IsLeftButtonPressed) + return; + + if (TryNavigateToNode(originNodeIds[0])) + e.Handled = true; + }; + } + /// /// #436: marks the warnings SQL Server itself wrote into the plan, so they are not read as one of /// our inferences. Only the engine's are tagged — they are the minority, and a badge on every diff --git a/src/PlanViewer.Core/Models/PlanModels.cs b/src/PlanViewer.Core/Models/PlanModels.cs index 20ecd09..d6341d0 100644 --- a/src/PlanViewer.Core/Models/PlanModels.cs +++ b/src/PlanViewer.Core/Models/PlanModels.cs @@ -391,6 +391,21 @@ public class PlanWarning /// public PlanWarningSource Source { get; set; } = PlanWarningSource.PerformanceStudio; + /// + /// The operators this finding actually came from, so a reader can be taken to them (#440). + /// + /// A LIST rather than a single id, because the three honest answers are genuinely + /// different. A key lookup came from exactly one operator. A table variable warning came from + /// every operator that touched one, which on a big plan is several. And some findings have no + /// operator at all — "High Compile CPU" happened before a single row was read, and + /// "UDF Execution" is reported by SQL Server at the statement level only. Those keep this + /// empty, and the UI offers no navigation rather than picking somewhere arbitrary. + /// + /// Sending a reader to the wrong operator is worse than sending them nowhere, because + /// they would believe it. + /// + public List OriginNodeIds { get; set; } = []; + /// /// Maximum percentage of elapsed time that could be saved by addressing this finding. /// null = not quantifiable, 0 = calculated as negligible. diff --git a/src/PlanViewer.Core/Output/AnalysisResult.cs b/src/PlanViewer.Core/Output/AnalysisResult.cs index 267f4fc..c9a3e70 100644 --- a/src/PlanViewer.Core/Output/AnalysisResult.cs +++ b/src/PlanViewer.Core/Output/AnalysisResult.cs @@ -256,6 +256,15 @@ public class WarningResult /// [JsonPropertyName("source")] public string Source { get; set; } = ""; + + /// + /// Node ids of the operators this finding came from, empty when it has no operator origin — + /// "High Compile CPU" happened before any operator ran (#440). Distinct from + /// , which says where the warning is ATTACHED in this output; these say + /// where it came FROM, and for a statement-level warning there is no NodeId at all. + /// + [JsonPropertyName("origin_node_ids")] + public List OriginNodeIds { get; set; } = []; } public class MissingIndexResult diff --git a/src/PlanViewer.Core/Output/ResultMapper.cs b/src/PlanViewer.Core/Output/ResultMapper.cs index 97996af..83ac5fd 100644 --- a/src/PlanViewer.Core/Output/ResultMapper.cs +++ b/src/PlanViewer.Core/Output/ResultMapper.cs @@ -196,7 +196,8 @@ private static StatementResult MapStatement( MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, IsLegacy = w.IsLegacy, - Source = w.Source.ToString() + Source = w.Source.ToString(), + OriginNodeIds = w.OriginNodeIds }); } @@ -306,7 +307,8 @@ private static OperatorResult MapNode(PlanNode node, CancellationToken cancellat MaxBenefitPercent = w.MaxBenefitPercent, ActionableFix = w.ActionableFix, IsLegacy = w.IsLegacy, - Source = w.Source.ToString() + Source = w.Source.ToString(), + OriginNodeIds = w.OriginNodeIds }); } diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs index 29086f6..ce63fa3 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs @@ -21,12 +21,18 @@ private static bool HasBatchModeNode(PlanNode node) return false; } + /* #440: collects the operators it found, because this walk already knows exactly which ones + touched a table variable and used to throw that away. Two lists rather than one, since the + two warnings this feeds are about different operators: every operator referencing a table + variable, versus only the ones modifying it (which is what forces the plan serial). */ private static void CheckForTableVariables(PlanNode node, bool isModification, - ref bool hasTableVar, ref bool modifiesTableVar) + ref bool hasTableVar, ref bool modifiesTableVar, + List? referencingNodeIds = null, List? modifyingNodeIds = null) { if (!string.IsNullOrEmpty(node.ObjectName) && node.ObjectName.StartsWith("@")) { hasTableVar = true; + referencingNodeIds?.Add(node.NodeId); // The modification target is typically an Insert/Update/Delete operator on a table variable if (isModification && (node.PhysicalOp.Contains("Insert", StringComparison.OrdinalIgnoreCase) || node.PhysicalOp.Contains("Update", StringComparison.OrdinalIgnoreCase) @@ -34,10 +40,12 @@ private static void CheckForTableVariables(PlanNode node, bool isModification, || node.PhysicalOp.Contains("Merge", StringComparison.OrdinalIgnoreCase))) { modifiesTableVar = true; + modifyingNodeIds?.Add(node.NodeId); } } foreach (var child in node.Children) - CheckForTableVariables(child, isModification, ref hasTableVar, ref modifiesTableVar); + CheckForTableVariables(child, isModification, ref hasTableVar, ref modifiesTableVar, + referencingNodeIds, modifyingNodeIds); } /// diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs index 2a702bf..8dcbbc2 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Node.cs @@ -48,6 +48,20 @@ private static void AnalyzeNode(PlanNode node, PlanStatement stmt, AnalyzerConfi Rule28_RowCountSpool(node, stmt, cfg); Rule29_ImplicitConversionSeek(node, stmt, cfg); Rule35_ExpensiveOperator(node, stmt, cfg); + + /* #440: an operator warning's origin is the operator it is hanging off, so it is stamped + here rather than at each of the 26 places above that add one. Same reasoning as the + provenance stamp in ShowPlanParser: a rule you have to remember at every construction + site is a rule that eventually gets forgotten, and the one time it is forgotten the UI + quietly offers no link on a warning that has a perfectly good one. + + Only fills what a rule left empty, so a rule that knows better - one pointing at the + operator that CAUSED the problem rather than the one reporting it - keeps its own answer. */ + foreach (var warning in node.Warnings) + { + if (warning.OriginNodeIds.Count == 0) + warning.OriginNodeIds.Add(node.NodeId); + } } private static void Rule01_FilterOperator(PlanNode node, PlanStatement stmt, AnalyzerConfig cfg) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs index 901e5ee..bce1d0c 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Statement.cs @@ -488,7 +488,10 @@ private static void Rule22Stmt_TableVariable(PlanStatement stmt, AnalyzerConfig var hasTableVar = false; var isModification = stmt.StatementType is "INSERT" or "UPDATE" or "DELETE" or "MERGE"; var modifiesTableVar = false; - CheckForTableVariables(stmt.RootNode, isModification, ref hasTableVar, ref modifiesTableVar); + var referencingNodeIds = new List(); + var modifyingNodeIds = new List(); + CheckForTableVariables(stmt.RootNode, isModification, ref hasTableVar, ref modifiesTableVar, + referencingNodeIds, modifyingNodeIds); if (hasTableVar && !modifiesTableVar) { @@ -496,7 +499,8 @@ private static void Rule22Stmt_TableVariable(PlanStatement stmt, AnalyzerConfig { WarningType = "Table Variable", Message = "Table variable detected. Table variables lack column-level statistics, which causes bad row estimates, join choices, and memory grant decisions. Replace with a #temp table.", - Severity = PlanWarningSeverity.Warning + Severity = PlanWarningSeverity.Warning, + OriginNodeIds = referencingNodeIds }); } @@ -506,7 +510,8 @@ private static void Rule22Stmt_TableVariable(PlanStatement stmt, AnalyzerConfig { WarningType = "Table Variable", Message = "This query modifies a table variable, which forces the entire plan to run single-threaded. SQL Server cannot use parallelism for modifications to table variables. Replace with a #temp table to allow parallel execution.", - Severity = PlanWarningSeverity.Critical + Severity = PlanWarningSeverity.Critical, + OriginNodeIds = modifyingNodeIds }); } } diff --git a/src/PlanViewer.Core/Services/WarningIndex.cs b/src/PlanViewer.Core/Services/WarningIndex.cs new file mode 100644 index 0000000..5d4f8d6 --- /dev/null +++ b/src/PlanViewer.Core/Services/WarningIndex.cs @@ -0,0 +1,41 @@ +using System.Collections.Generic; +using PlanViewer.Core.Models; + +namespace PlanViewer.Core.Services; + +/// +/// Gathers the warnings scattered across an operator tree into one list (#440). +/// +/// Lives in Core rather than next to the panel that renders it for two reasons: it is a plan +/// tree walk over Core's own models with nothing UI about it, and putting it here means it can be +/// tested without standing up Avalonia. +/// +public static class WarningIndex +{ + /// + /// Every warning hanging off an operator beneath , paired with the + /// operator carrying it, in no particular order — callers sort for themselves, because the + /// statement panel wants them by benefit while other callers may not. + /// + public static List<(PlanNode Node, PlanWarning Warning)> CollectOperatorWarnings(PlanNode? root) + { + var collected = new List<(PlanNode, PlanWarning)>(); + if (root == null) + return collected; + + /* Explicit stack rather than recursion: a deep plan is exactly the case this feature exists + for, and #430 was a crash caused by assuming operator trees are shallow. */ + var pending = new Stack(); + pending.Push(root); + while (pending.Count > 0) + { + var node = pending.Pop(); + foreach (var warning in node.Warnings) + collected.Add((node, warning)); + foreach (var child in node.Children) + pending.Push(child); + } + + return collected; + } +} diff --git a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs index 5be64e4..d0d809a 100644 --- a/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs +++ b/tests/PlanViewer.Core.Tests/HistoricalCliContractTests.cs @@ -6,13 +6,15 @@ namespace PlanViewer.Core.Tests; public sealed class HistoricalCliContractTests { - /* Rolled for #436, which adds "source" to every warning in the JSON output so a consumer can tell - SQL Server's own warnings from Performance Studio's inferences. The change is additive — nothing - was removed or renamed, so a consumer reading fields by name is unaffected — but anything - diffing or hashing whole output sees different bytes, which is exactly what this constant is - here to make somebody decide on rather than discover. */ + /* Rolled twice now, both times additively and both times deliberately: #436 added "source" to + every warning so a consumer can tell SQL Server's own warnings from Performance Studio's + inferences, and #440 added "origin_node_ids" so a consumer can get from a finding to the + operator that produced it. Nothing has been removed or renamed either time, so a consumer + reading fields by name is unaffected — but anything diffing or hashing whole output sees + different bytes, which is exactly what this constant exists to make somebody decide on rather + than discover. Verified for #440 that the only new key on a warning is origin_node_ids. */ private const string ExpectedCompactOutputSha256 = - "06975e4e513eef23669c86bc2cfeec916f442461d605060bbd996bd44df13405"; + "779a85036b74dc7cc7d26d4bad3d7e2b327e4ff89a5ed79b00b78cfae3717cab"; [Fact] public async Task AnalyzeCompact_PreservesHistoricalOutputBytes() diff --git a/tests/PlanViewer.Core.Tests/WarningOriginTests.cs b/tests/PlanViewer.Core.Tests/WarningOriginTests.cs new file mode 100644 index 0000000..a0f1651 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/WarningOriginTests.cs @@ -0,0 +1,144 @@ +using System.Linq; +using PlanViewer.Core.Models; +using PlanViewer.Core.Output; + +namespace PlanViewer.Core.Tests; + +/// +/// #440: a warning did not record which operator it came from, so on a large plan there was no way +/// to get from a finding to the thing that produced it. +/// +/// The interesting half of this feature is knowing when to say nothing. A warning pointed at the +/// wrong operator is worse than a warning pointed nowhere, because the reader would believe it — so +/// these pin both directions: that origins appear where a rule genuinely knows, and that they stay +/// empty where no operator is responsible. +/// +public class WarningOriginTests +{ + /// + /// Operator warnings are stamped in one place, at the end of AnalyzeNode, rather than at the 26 + /// separate sites that add one. This asserts the property that arrangement buys: across every + /// committed plan, no operator warning is ever left without an origin. + /// + [Fact] + public void EveryOperatorWarningKnowsItsOperator() + { + var plansDir = System.IO.Path.Combine(System.AppContext.BaseDirectory, "Plans"); + var orphans = new System.Collections.Generic.List(); + + foreach (var file in System.IO.Directory.GetFiles(plansDir, "*.sqlplan")) + { + var plan = PlanTestHelper.LoadAndAnalyze(System.IO.Path.GetFileName(file)); + foreach (var stmt in plan.Batches.SelectMany(b => b.Statements)) + { + if (stmt.RootNode == null) continue; + foreach (var (node, warning) in Walk(stmt.RootNode)) + { + if (warning.OriginNodeIds.Count == 0) + orphans.Add($"{System.IO.Path.GetFileName(file)}:{warning.WarningType}"); + else if (!warning.OriginNodeIds.Contains(node.NodeId)) + orphans.Add($"{System.IO.Path.GetFileName(file)}:{warning.WarningType} points away from its own node"); + } + } + } + + Assert.True(orphans.Count == 0, "Operator warnings without a usable origin: " + string.Join(", ", orphans)); + } + + /// + /// The statement-level table variable warning is the case that motivated carrying origins on + /// statement warnings at all: the rule already walked the tree and knew exactly which operators + /// touched a table variable, and threw that away before emitting. + /// + [Fact] + public void TheTableVariableWarningPointsAtTheOperatorsThatTouchOne() + { + var plan = PlanTestHelper.LoadAndAnalyze("table_variable_plan.sqlplan"); + var statementWarnings = plan.Batches + .SelectMany(b => b.Statements) + .SelectMany(s => s.PlanWarnings) + .Where(w => w.WarningType == "Table Variable") + .ToList(); + + Assert.NotEmpty(statementWarnings); + Assert.All(statementWarnings, w => Assert.NotEmpty(w.OriginNodeIds)); + } + + /// + /// The other direction, and the one worth protecting. "High Compile CPU" is measured before a + /// single row is read, so no operator is responsible for it; SQL Server reports "UDF Execution" + /// at the statement level only. Both must stay empty so the UI offers no link rather than a + /// misleading one. + /// + [Theory] + [InlineData("convert_implicit_plan.sqlplan", "High Compile CPU")] + [InlineData("udf_plan.sqlplan", "UDF Execution")] + public void WarningsWithNoResponsibleOperatorClaimNone(string planFile, string warningType) + { + var plan = PlanTestHelper.LoadAndAnalyze(planFile); + var matching = plan.Batches + .SelectMany(b => b.Statements) + .SelectMany(s => s.PlanWarnings) + .Where(w => w.WarningType == warningType) + .ToList(); + + Assert.NotEmpty(matching); + Assert.All(matching, w => Assert.Empty(w.OriginNodeIds)); + } + + /// The JSON consumers get origins as a field, not by re-deriving them from the tree. + [Fact] + public void TheJsonOutputCarriesOrigins() + { + var plan = PlanTestHelper.LoadAndAnalyze("table_variable_plan.sqlplan"); + var result = ResultMapper.Map(plan, "table_variable_plan.sqlplan"); + + var tableVariable = result.Statements + .SelectMany(s => s.Warnings) + .Single(w => w.Type == "Table Variable"); + + Assert.NotEmpty(tableVariable.OriginNodeIds); + } + + /// + /// The index the statement panel is built from. The reporter's case is a plan too big to hunt + /// through by hand, so "found all of them" is the property that matters — this compares the + /// index against an independent recursive walk rather than against a hand-written expected + /// count, so it cannot drift with the fixtures. + /// + [Fact] + public void TheOperatorWarningIndexFindsEveryWarningInTheTree() + { + var plansDir = System.IO.Path.Combine(System.AppContext.BaseDirectory, "Plans"); + var mismatches = new System.Collections.Generic.List(); + + foreach (var file in System.IO.Directory.GetFiles(plansDir, "*.sqlplan")) + { + var plan = PlanTestHelper.LoadAndAnalyze(System.IO.Path.GetFileName(file)); + foreach (var stmt in plan.Batches.SelectMany(b => b.Statements)) + { + if (stmt.RootNode == null) continue; + var indexed = PlanViewer.Core.Services.WarningIndex.CollectOperatorWarnings(stmt.RootNode).Count; + var walked = Walk(stmt.RootNode).Count(); + if (indexed != walked) + mismatches.Add($"{System.IO.Path.GetFileName(file)}: indexed {indexed} vs walked {walked}"); + } + } + + Assert.True(mismatches.Count == 0, string.Join(", ", mismatches)); + } + + /// A statement with no operator tree indexes to nothing rather than throwing. + [Fact] + public void TheOperatorWarningIndexHandlesAMissingTree() + { + Assert.Empty(PlanViewer.Core.Services.WarningIndex.CollectOperatorWarnings(null)); + } + + private static System.Collections.Generic.IEnumerable<(PlanNode Node, PlanWarning Warning)> Walk(PlanNode node) + { + foreach (var w in node.Warnings) yield return (node, w); + foreach (var child in node.Children) + foreach (var pair in Walk(child)) yield return pair; + } +}