Skip to content

Parse IF-condition query plans and MULTIPLE PLAN hashes (#580) - #581

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/parser-cond-multiple-plan-580
Sep 27, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/parser-cond-multiple-plan-580

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 27, 2026 •

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes #580. The plan parser dropped two statement shapes that carry their own plan or hashes. PerformanceMonitor fixed the same two gaps in its copy of the parser in erikdarlingdata/PerformanceMonitor#4470. This PR ports that fix to Studio's parser.

  1. An IF or WHILE condition that runs a query (StmtCond with StatementType="COND WITH QUERY"). The showplan schema puts the condition's own QueryPlan under <Condition>, next to optional UDF sub-plans. The parser treated each child of <Condition> as a statement. The bare QueryPlan became an empty STATEMENT placeholder, and the condition's operator tree, hashes, missing indexes and warnings were lost. Each UDF child became another empty placeholder, and the function body was lost.
    The parser now reads the StmtCond as the statement, and it reads the plan and the sub-plans from <Condition>. ParseStatement has a new optional planContainerEl argument for this. A UDF body attaches to the condition statement through ParseSubPlans, as it does for every other statement. A plain IF with no query and no UDF still adds no statement.
  2. A MULTIPLE PLAN statement (StmtSimple with StatementType="MULTIPLE PLAN"). It carries QueryHash and QueryPlanHash but no QueryPlan. ParseStatement returned at the no-plan branch before it read the statement attributes. It now reads them first. Every statement without a plan now gets StatementId, its hashes and the other statement attributes. Examples are DECLARE, ASSIGN and EXEC. The properties panel shows those hashes, and plan comparison can pair such statements by hash.

One difference from #4470: PerformanceMonitor adds a condition's UDF statements to the top-level statement list. Studio attaches them to the condition statement, because Studio attaches every module body to its calling statement (#456, #486, #491).

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

The app, the CLI, the web viewer and the MCP tools all read the parser's output, so they all show the condition statement now. None of their code changed.

How was this tested?

  • New ShowPlanParserCondAndMultiplePlanTests (7 tests). Five come from #4470, with its repro XML. Two are Studio's own: a UDF sub-plan under <Condition> attaches to the condition statement, and a plain IF adds no statement. Against the unfixed parser, 4 of the 7 fail. The other 3 cover behavior that was already correct: the statement count, the Then branch and the plain IF.
  • Full suite before the baseline update: 786 passed, 2 failed, 1 skipped. The 2 failures were the two golden-file tests. Both changes come from this fix:
    • ComparisonBaseline.txt: eager_table_spool_plan.sqlplan has a WHILE (SELECT ...) loop. Its condition used to be an empty statement with no text, at the end of the list. Now it is a real statement with its text, cost and row estimate. The comparison pairs it by query hash, so it moves to Statement 1 and the other statements move down by one.
    • WarningBaseline.txt: one new warning, on the same WHILE condition. Rule 23 (table-valued function) flags the sys.dm_db_index_physical_stats call (INDEXANALYSIS) in the condition's plan. The analyzer did not see that plan before, and rule 23 did not change. The rule's advice to rewrite as an inline function does not fit a system function. Whether rule 23 should skip built-in functions is a separate question, outside this fix.
  • After the baseline update, the two golden tests and the 7 new tests pass (9 of 9). The Release build has 0 compiler warnings.
  • Plans tested: synthetic estimated-plan XML and the 60 committed fixture plans, on Windows.

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (Release build through dotnet test)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR

A StmtCond keeps its condition's own QueryPlan (and any UDF sub-plans)
under <Condition>. The parser fed each child of <Condition> back in as a
statement, so the condition became an empty STATEMENT placeholder and its
operator tree, hashes, missing indexes and warnings were lost. Parse the
StmtCond itself as the statement and read its plan and sub-plans from
<Condition> (new optional planContainerEl argument on ParseStatement).

ParseStatement read the statement attributes only after the no-QueryPlan
return, so a MULTIPLE PLAN statement lost the QueryHash and QueryPlanHash
it carries. Read them before that return.

Port of erikdarlingdata/PerformanceMonitor#4470. The two golden baselines
change because eager_table_spool_plan.sqlplan has a WHILE (SELECT ...)
condition that is now a real statement.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G625JBNh45iTR1hpT4CxNR
@claude

claude Bot commented Sep 27, 2026

Copy link
Copy Markdown

Reviewed. This is a clean, well-scoped port of the PerformanceMonitor fix (#4470):

  • The StmtCond/Condition attribute sourcing checks out against the XSD as described — StatementSetOptions, RetrievedFromCache, hashes, and StatementText correctly stay read from stmtEl (the StmtCond), while QueryPlan/MemoryGrantInfo/UDF sub-plans correctly move to planContainerEl (Condition).
  • The new ParseStatement(stmtEl, depth, ..., planContainerEl: condEl) call keeps the MaxParseDepth guard intact: UDF sub-plans under Condition still recurse through ParseStatementAndChildren at depth + 1 via ParseSubPlans, so a maliciously nested Condition > UDF > StmtCond > Condition > UDF... chain is still bounded the same as before.
  • MULTIPLE PLAN now reads ParseStmtAttributes before the no-QueryPlan early return, matching the existing pattern used for DECLARE/ASSIGN.
  • New tests exercise both ported scenarios plus two Studio-specific ones (UDF-under-Condition attaching to the condition statement, and a plain IF adding no statement), and the golden-file diffs (ComparisonBaseline.txt/WarningBaseline.txt) match the behavior change described in the PR body.

No correctness, untrusted-input, security, or repo-convention issues found (no Directory.Build.props/SSMS version files touched, no new PlanViewer.Core file needing a PlanViewer.Web.csproj linked include, no T-SQL generation involved).

@erikdarlingdata
erikdarlingdata merged commit 1ae7da5 into dev Sep 27, 2026
4 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/parser-cond-multiple-plan-580 branch September 27, 2026 23:29
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