Repository navigation
Plan analysis: report which operator each finding came from (#4534) - #4556
Merged
Merged
Conversation
…io's own test plans (#4534)
…node-ids # Conflicts: # PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs
erikdarlingdata
marked this pull request as ready for review
September 28, 2026 04:23
This was referenced Sep 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).McpPlanAnalysisFormatter's warning JSON carriesorigin_node_idsnext tosource(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
OriginNodeIdsto 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), plusPlanSync4534OriginFixturesTests— a second class that ports PerformanceStudio's ownWarningOriginTestsfacts against three of PerformanceStudio's real.sqlplanfixtures (table_variable_plan.sqlplan,convert_implicit_plan.sqlplan,udf_plan.sqlplan, copied verbatim from its public repo intoDarling/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 onconvert_implicit_planand UDF Execution onudf_planboth claim no operator origin; the MCP JSON output carriesorigin_node_idsfor 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.Testscannot run on macOS (discovery dies on WindowsBase); none of its classes touch plan analysis output shapes for this change, so none needed updating.Pins
PlanSync4534Testsbuilds 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'sOriginNodeIdsis 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.McpFormatter_CarriesOriginNodeIdsassertsBuildAnalysisResult's JSON containsorigin_node_idsand that the table-scan finding's value is[1].McpPlanAnalysisEnvelopeTestsstays green .RED before this fix: compile-only.
PlanSync4534Tests.cscopied ontoorigin/dev(detached worktree at the pre-fixPlanAnalyzer.cs/PlanModels.cs) fails to build withCS1061: 'PlanWarning' does not contain a definition for 'OriginNodeIds'at every assertion site — the member doesn't exist yet.Mutation: removed
OriginNodeIds = referencingNodeIdsfrom the "table variable detected" finding's construction (leaving the generic node-level stamp, which never runs for a statement-level finding).TableVariableReferenceWarning_OriginIsTheReferencingOperatorwent RED:Expected: [1], Actual: []. Reverted; rebuilt; the full run above is green again.Second mutation (
PlanSync4534OriginFixturesTests): disabled the generic per-node stamp itself (theforeach (var warning in node.Warnings) if (warning.OriginNodeIds.Count == 0) ...loop at the end ofAnalyzeNode) 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 sameAnalyzeNodemethod this branch's stamp loop appends to. Resolved with the stamp loop AFTER rule 35's block, mirroring PerformanceStudio's own order (Rule35_ExpensiveOperatorthen the stamp), so a rule-35 finding also gets an origin. AddedExpensiveOperatorWarning_OriginIsTheOperatorItsOwnNodetoPlanSync4534Tests: a single leaf Table Scan owning 60% of a 2,000ms statement's elapsed time with no other warning gets an "Expensive Operator" finding whoseOriginNodeIdsis[7](the scan's own node ID). RED: with the stamp loop moved ABOVE rule 35 inAnalyzeNode(temporary swap, reverted), the fact fails —Assert.Singlefinds no "Expensive Operator" warning has run yet to be stamped when the loop executes first, soOriginNodeIdsis 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/4534OriginFixturesplus the standing plan-analysis/MCP/doc-hygiene classes):Total: 305, Errors: 0, Failed: 0, Skipped: 2, Not Run: 0.Lite.TestsandDarling.Testsboth 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_idsin the MCP plan tools' JSON output.REF: [#4556]: #4556