Skip to content

Plan analysis: report which operator each finding came from (#4534) - #4556

Merged
erikdarlingdata merged 5 commits into
devfrom
plan-sync/4534-origin-node-ids
Sep 28, 2026
Merged

erikdarlingdata merged 5 commits into
devfrom
plan-sync/4534-origin-node-ids

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Fixes #4534
Part of #4511

Why

A finding did not record which operator produced it, so on a large plan there was no way to get from a finding to the thing that caused it.

What changes

Mirrors erikdarlingdata/PerformanceStudio@9c3fceb (PerformanceStudio#440/#446):

  • PlanWarning.OriginNodeIds: a list of node IDs the finding came from. Empty when a finding has no operator origin (for example, High Compile CPU is measured before a single row is read).
  • Operator-level findings are stamped in one place, at the end of the per-node analysis pass, with the ID of the node they hang off — rather than at every construction site. A rule that already knows the operator that caused the problem (rather than the one reporting it) keeps its own answer; the generic stamp only fills what a rule left empty.
  • The table variable rule already walked the plan tree looking for every operator that references or modifies a table variable, and used to throw that list away before emitting the two findings it can produce. It now keeps it: the "table variable detected" finding lists every referencing operator, and the "modifies a table variable" finding lists only the operators that force the plan single-threaded.
  • McpPlanAnalysisFormatter's warning JSON carries origin_node_ids next to source (added in Plan analysis: mark which warnings are SQL Server's and key duplicate index suggestions on the database (#4520, #4523) #4543), additive.

Not ported: the two PerformanceStudio viewer sites that use OriginNodeIds to scroll a click to the responsible operator (PlanViewerControl.Properties.cs). PerformanceMonitor's plan viewer needs its own UI work to use the field; this PR only adds the data.

Test plan

PlanSync4534Tests (synthetic repro), plus PlanSync4534OriginFixturesTests — a second class that ports PerformanceStudio's own WarningOriginTests facts against three of PerformanceStudio's real .sqlplan fixtures (table_variable_plan.sqlplan, convert_implicit_plan.sqlplan, udf_plan.sqlplan, copied verbatim from its public repo into Darling/Darling.Tests/Fixtures/OriginPlans/, each run through scrubcheck clean before being added). Ported facts: every operator-level warning across the three fixtures carries its own node as an origin (EveryOperatorWarningKnowsItsOperator); the Table Variable statement warning on the real fixture is non-empty (TheTableVariableWarningPointsAtTheOperatorsThatTouchOne); High Compile CPU on convert_implicit_plan and UDF Execution on udf_plan both claim no operator origin; the MCP JSON output carries origin_node_ids for the real fixture's Table Variable warning. Plus the standing plan-analysis regression classes.

dotnet build Lite.Tests -c Release -p:EnableWindowsTargeting=true: 0 errors, 0 warnings. Lite.Tests cannot run on macOS (discovery dies on WindowsBase); none of its classes touch plan analysis output shapes for this change, so none needed updating.

Pins

  1. The IDs match the operators in the test plan. PlanSync4534Tests builds a synthetic plan with a SELECT statement reading a table variable (node 1) and an INSERT statement modifying one (node 10, inserting from a Table Scan at node 11). Assertions: the "table variable detected" finding's OriginNodeIds is exactly [1]; the "modifies a table variable" finding's is exactly [10], not [11]; every operator-level finding in the tree carries its own node's ID; the statement-level High Compile CPU finding carries none.
  2. The MCP output carries them. McpFormatter_CarriesOriginNodeIds asserts BuildAnalysisResult's JSON contains origin_node_ids and that the table-scan finding's value is [1]. McpPlanAnalysisEnvelopeTests stays green .

RED before this fix: compile-only. PlanSync4534Tests.cs copied onto origin/dev (detached worktree at the pre-fix PlanAnalyzer.cs/PlanModels.cs) fails to build with CS1061: 'PlanWarning' does not contain a definition for 'OriginNodeIds' at every assertion site — the member doesn't exist yet.

Mutation: removed OriginNodeIds = referencingNodeIds from the "table variable detected" finding's construction (leaving the generic node-level stamp, which never runs for a statement-level finding). TableVariableReferenceWarning_OriginIsTheReferencingOperator went RED: Expected: [1], Actual: []. Reverted; rebuilt; the full run above is green again.

Second mutation (PlanSync4534OriginFixturesTests): disabled the generic per-node stamp itself (the foreach (var warning in node.Warnings) if (warning.OriginNodeIds.Count == 0) ... loop at the end of AnalyzeNode) by short-circuiting its condition. EveryOperatorWarningKnowsItsOperator (running over the three real PerformanceStudio fixtures) went RED with operator-level findings reported as having no usable origin. Reverted; rebuilt; the full run above is green again.

Ordering with rule 35 (#4550): merged origin/dev, which added rule 35 in the same AnalyzeNode method this branch's stamp loop appends to. Resolved with the stamp loop AFTER rule 35's block, mirroring PerformanceStudio's own order (Rule35_ExpensiveOperator then the stamp), so a rule-35 finding also gets an origin. Added ExpensiveOperatorWarning_OriginIsTheOperatorItsOwnNode to PlanSync4534Tests: a single leaf Table Scan owning 60% of a 2,000ms statement's elapsed time with no other warning gets an "Expensive Operator" finding whose OriginNodeIds is [7] (the scan's own node ID). RED: with the stamp loop moved ABOVE rule 35 in AnalyzeNode (temporary swap, reverted), the fact fails — Assert.Single finds no "Expensive Operator" warning has run yet to be stamped when the loop executes first, so OriginNodeIds is checked against an empty warnings collection at that point (the loop runs before rule 35 adds anything). Reverted the swap; rebuilt; the targeted run below is green again.

Run (with current dev merged) (PlanSync4513/4515/4515b/4520-4529/4534/4534OriginFixtures plus the standing plan-analysis/MCP/doc-hygiene classes): Total: 305, Errors: 0, Failed: 0, Skipped: 2, Not Run: 0. Lite.Tests and Darling.Tests both build 0 errors / 0 warnings with -p:EnableWindowsTargeting=true.

CHANGELOG

SECTION: Added
ENTRY: - The MCP plan tools report which operator each finding came from ([#4556]) - Every plan analysis finding now carries the node IDs of the operators it came from, empty for findings with no single operator responsible (such as high compile CPU). Carried in origin_node_ids in the MCP plan tools' JSON output.
REF: [#4556]: #4556

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 04:23
@erikdarlingdata
erikdarlingdata merged commit 10920cb into dev Sep 28, 2026
15 of 16 checks passed
@erikdarlingdata
erikdarlingdata deleted the plan-sync/4534-origin-node-ids branch September 28, 2026 04:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant