Say which warnings are SQL Server's and which are ours (#436) - #439
Merged
Merged
Conversation
The reporter could not tell our advice apart from the engine's, and there was no way to: both arrive as a PlanWarning carrying nothing but type, severity and message, and both render identically. 14 warning constructions in ShowPlanParser.Warnings.cs are lifted straight out of the plan's own <Warnings> element; 47 across PlanAnalyzer*.cs are our rules reading plan shape. Nothing in the model or the output said which was which. The distinction is not cosmetic. A warning SQL Server wrote into the plan is a record of what the engine did — it spilled, it converted, it had no statistics. One of our rules is an inference, and an inference can be wrong about a particular plan in a way the engine's own record cannot be. The same issue supplies the example: we told this reporter a conversion prevented an index seek on a plan SQL Server had raised no conversion warning about at all, and being able to see "that line is ours, this line is the engine's" is what would have let him weigh the two himself. PlanWarning.Source defaults to PerformanceStudio, and everything the parser produces is stamped SqlServer in ONE place — the single return of ParseWarningsFromElement, which every parser warning already funnels through. Stamping the 14 construction sites individually would have been the same bug as #430's missed QueryStoreCommand: a rule you have to remember at each site is a rule that eventually gets forgotten. A new engine warning added to that method is attributed correctly without anyone thinking about it. UX, since it was left to me: - Only the ENGINE's warnings are tagged, as " [SQL Server]", in both the GUI warnings panel and the CLI text output. Tagging both kinds would put a badge on every line and carry no information — our own advice is what a reader already expects from a plan analyzer, so the marked case should be the exception. - The tag sits next to the existing " [legacy]" tag and is built the same way, because that badge already established the idiom here. - JSON and MCP consumers get it as a "source" field rather than having to parse a tag out of a string. CLI output contract, flagged rather than slipped in: adding "source" to every warning changes the bytes of `analyze --compact`, so HistoricalCliContractTests.ExpectedCompactOutputSha256 is rolled. The change is additive — nothing removed or renamed, so a consumer reading fields by name is unaffected — but anything hashing or diffing whole output will see a difference. That constant exists to make this a decision instead of a discovery, so it is called out here rather than quietly updated. The characterization baseline is untouched: it digests type, severity and message, none of which changed. Tested: 65 across WarningSourceTests, HistoricalCliContractTests, WarningCharacterizationTests and PlanAnalyzerTests, 0 failures. The new tests pin both kinds on one plan that carries both, that no warning type is ever produced as both kinds across every committed plan, that only the engine's are tagged in text output, and that the JSON field is populated. dotnet build clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 of 5 tasks
|
Reviewed. The core mechanism is solid: all 14 One real gap: the PR's stated goal is to tag the engine's warnings "in both the GUI warnings panel and the CLI text output," but there are two more rendering surfaces that already carry the analogous
Neither is a correctness bug — No correctness, security, or untrusted-input issues found in the warning-attribution logic itself. |
Merged
erikdarlingdata
added a commit
that referenced
this pull request
Aug 21, 2026
Minor rather than patch. 1.19.x would understate it: #439 adds a "source" field to every warning in the JSON and MCP output and a new badge in the app and CLI, and #437 changes what an existing analysis rule concludes about a plan. Both are things a consumer can notice, and one of them is output-shape. What ships: - #437 Rule 12 no longer calls a conversion non-SARGable when it converts the parameter rather than the column. Plans carrying a parameter-side conversion on a scan lose that warning and report the scan's residual predicate instead. Verified against all 38 committed plans: no other plan's verdict moves. - #439 SQL Server's own warnings are now told apart from ours, tagged [SQL Server] in the app and CLI and carried as "source" in JSON/MCP. Additive, but it changes the bytes of analyze --compact. - #431 Robot Advice no longer takes the app down on a deep plan. - #438 querystore gets the same depth ceiling analyze got; it had been failing quietly on deep plans, one ERROR row per plan. - #443 the macOS handle resolver no longer corrupts memory on Apple silicon. - #425 Entra MFA works again (WAM parent window handle). Not user-facing but worth knowing for anyone building from this tag: the suite runs on Microsoft.Testing.Platform now (#442), and `dotnet test` finishes on macOS for the first time (#443) - 307 tests, 305 passing, 13 seconds. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 21, 2026
Merged
erikdarlingdata
added a commit
that referenced
this pull request
Aug 21, 2026
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) <noreply@anthropic.com>
erikdarlingdata
added a commit
that referenced
this pull request
Aug 21, 2026
…#449) UpdateCompareButtonState counted plans in the session's OWN sub-tabs, so two queries in two separate sessions - one plan each - left the button disabled in both, which is precisely the comparison it exists for. The plans were always reachable. MainWindow.CollectAllPlanTabs spans sessions and labels them "Query 1 > Plan", and the file-mode Compare button has always used it. That is why the reporter's workaround worked: saving a plan and reopening it gave him a file tab, whose button looks at the window-wide collection. Only the session button was looking at the wrong one, in both its enablement and its own narrower picker. So the session button now asks the window for both, and hands off to the window's picker rather than keeping a second one that cannot see past its own session. The refresh is ONE subscription to MainTabControl.Items rather than a call added at each of the sixteen places that add or remove a tab. That is the same reasoning as #438, #439 and #440, and it matters more here than usual: a plan appearing in one session changes whether Compare is available in every OTHER session, so the refresh has to be window-wide and a call site that gets forgotten leaves a stale button somewhere the author never looked. Kept a fallback to the session's own count for when there is no owning MainWindow - the control not yet attached, or hosted somewhere else - so the button is never left in a stale state rather than throwing. Tooltip updated, because it said "Compare two plan tabs" and now means something wider. No automated test, and I would rather say so than pretend. This is UI wiring and the repo has no headless Avalonia harness; a test that would have caught it needs two constructed sessions and a window. Building that harness for one bug is disproportionate, and putting Avalonia into the test process is not something to do casually given the test-host wedge in #441. A test on the arithmetic would be hollow - count >= 2 was never the broken part, the SCOPE was. Verifiable without a SQL Server, which is worth recording because two sessions holding real plans otherwise need a live connection: open two .sqlplan files, then New Query. The session's Compare button is enabled and its picker lists both file plans. On dev it stays disabled, because the session has no plans of its own and IsEnabled="False" is the XAML default. Tested: 314 total, 312 passed, 0 failed - unchanged, since nothing here touches Core. App builds clean and runs without exceptions on two plan tabs. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
erikdarlingdata
added a commit
that referenced
this pull request
Aug 21, 2026
…#450) A failed query reported an error that was cut off, and it was cut off three separate times on the way to the screen: - ex.Message[..100] + "..." at four sites in Execution.cs. A hundred characters does not even clear "Msg 208, Level 16, State 1, Server X, Line 1" before the sentence naming the actual problem starts, so the truncation reliably removed the only part worth reading. - statusLabel had no TextWrapping, so it defaulted to NoWrap and clipped whatever survived the truncation. - loadingPanel is a fixed Width = 300, sized for a spinner and a Cancel button rather than for prose. Any one of those alone would have cut a real SQL error. Together they made the message close to useless, which matches the report. All four catch sites now go through one helper rather than repeating the display logic - the same reasoning as #438, #439 and #440, and the reason it matters here is that the four sites were already identical and already wrong in the same way, which is what a copied line does over time. The helper widens the panel on failure (MaxWidth rather than Width, so a short error stays compact and a long one is bounded at a readable measure instead of running the whole window), wraps, and colours it as an error. The label is now a SelectableTextBlock. A SQL error is the one string in this app a user most needs to get out and paste somewhere else, and it could not be selected. Verified against a real server rather than by reading: SQL Server 2025 in Docker, and a query against a deliberately long object name produces a 191 character error where the old path stopped mid-word. Not fixed here, and flagged rather than folded in: QueryStore.cs:219 truncates at 80 characters before handing the text to a status bar that already does TextTrimming="CharacterEllipsis". Redundant and lossy, same family, but a different surface and no issue filed against it. No automated test. This is UI wiring and the repo has no headless Avalonia harness; a test that would have caught it needs a constructed control tree. Two bugs in one evening now sit in that gap, which is worth a decision about Avalonia.Headless rather than a hollow test asserting that a string is not truncated by code that no longer truncates it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
erikdarlingdata
added a commit
that referenced
this pull request
Aug 22, 2026
An EXEC <procedure> plan analyzed as one statement, no warnings, cost 0, exit 0, on a file carrying dozens of statement plans. Reproduced against SQL Server 2025 before touching anything: six StmtSimple, four QueryPlan, summed cost 1.88, and `analyze` reported total_statements 1 and max_estimated_cost 0. The reported diagnosis was that the parse never descends into the procedure. It is subtler than that, and the distinction is the fix. ShowPlanParser has ALWAYS read StoredProc sub-plans - but that code sits below an early return taken when a statement carries no QueryPlan of its own, and an EXEC statement is precisely a statement with no plan of its own, because every plan lives in the body. The descent existed and was unreachable in the only case it was written for. The same was true of a UDF call whose calling statement carries no plan. So the sub-plan parsing moves above that early return. That alone fixes it. Two more places had the same blind spot and are now sharing one traversal, because the traversal was never the missing part - PlanOperations.ValidateComplexity has always descended, which is how the complexity limit counted statements the analysis never saw: - PlanAnalyzer walked batch.Statements, so no rule ever ran on a procedure body. - ResultMapper walked batch.Statements, which is where total_statements 1 and max_estimated_cost 0 came from. And a third, which is the one worth pausing on: PlanTestHelper.AllWarnings walked batch.Statements too. The golden master and the analyzer shared a blind spot, so the characterization test could not have caught the analyzer skipping procedure bodies no matter how many procedure plans were committed. A test that cannot see what the code cannot see is not covering it. It now uses the same traversal. What this does NOT change: no committed plan's verdict moves. Regenerating WarningBaseline.txt across the corpus produces additions only - the new fixture and nothing else - because the fix only ever adds statements that were being dropped. The CLI output hash is unchanged for the same reason: a plain batch enumerates exactly as before. Also caught on the way in, and worth knowing: PlanViewer.Web compiles Core sources through an explicit file list rather than a glob, so a new Core file breaks the solution build until it is added there. It is the same shape of trap as the call sites in #438 and #439 - something you must remember at a second location - and I walked into it. Tested: 327 passing, 0 failed. The new tests fail against the original parser - three of them, exactly the three asserting the body is reached, while the ordering test and the unchanged-plan cases correctly still pass. Verified by reverting the parser rather than assumed. Reported by samplesty, with a genuinely good writeup: file statistics, the contrast against StmtCond working correctly, and the observation that the output is plausible rather than obviously broken, which is what makes it worth fixing rather than documenting. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
The second half of #436 — telling SQL Server's warnings apart from ours. Independent of #436's rule fix; either can merge without the other.
The problem
Both kinds arrive as a
PlanWarningcarrying nothing but type, severity and message, and both render identically. 14 warning constructions inShowPlanParser.Warnings.csare lifted straight out of the plan's own<Warnings>element; 47 acrossPlanAnalyzer*.csare our rules reading plan shape. Nothing in the model or the output said which was which.It isn't cosmetic. A warning SQL Server wrote into the plan is a record of what the engine did — it spilled, it converted, it had no statistics. One of our rules is an inference, and an inference can be wrong about a particular plan in a way the engine's own record cannot be.
The same issue supplies the example: we told this reporter a conversion prevented an index seek on a plan SQL Server had raised no conversion warning about. Being able to see "that line is ours, this line is the engine's" is what would have let him weigh the two himself.
How it's attributed
PlanWarning.Sourcedefaults toPerformanceStudio, and everything the parser produces is stampedSqlServerin one place — the single return ofParseWarningsFromElement, which every parser warning already funnels through.Stamping the 14 construction sites individually would have been the same bug as the one in #430's original fix: a rule you have to remember at each site is a rule that eventually gets forgotten. A new engine warning added to that method is attributed correctly without anyone thinking about it.
UX (left to me, so: what and why)
[SQL Server], in both the GUI warnings panel and the CLI text output. Tagging both kinds would put a badge on every line and carry no information — our own advice is what a reader already expects from a plan analyzer, so the marked case should be the exception. Easy to invert if you disagree.[legacy]tag and is built the same way, since that badge already established the idiom.sourcefield rather than having to parse a tag out of a string.CLI output contract — flagged, not slipped in
Adding
sourceto every warning changes the bytes ofanalyze --compact, soHistoricalCliContractTests.ExpectedCompactOutputSha256is rolled. The change is additive — nothing removed or renamed, so a consumer reading fields by name is unaffected — but anything hashing or diffing whole output sees a difference. That constant exists to make this a decision instead of a discovery, so it is called out rather than quietly updated.The characterization baseline is untouched: it digests type, severity and message, none of which changed.
Tests
New tests pin both kinds on one plan carrying both, that no warning type is ever produced as both kinds across every committed plan, that only the engine's are tagged in text output, and that the JSON field is populated. 65 tests, 0 failures.
dotnet buildclean.🤖 Generated with Claude Code