Repository navigation
Give querystore the depth ceiling that analyze got (#430) - #438
Merged
Merged
Conversation
#430 was fixed by making the depth ceiling a shared constant and referencing it from every serializer that writes an AnalysisResult. That enumeration missed two: QueryStoreCommand builds its own JsonOptions and CompactJsonOptions, identical in shape to AnalyzeCommand's, and neither ever got MaxDepth. So `planview querystore` has kept failing on any plan deeper than roughly 30 operators for as long as `analyze` has been fine. It fails quietly, which is why nobody has reported it. The per-plan try/catch in the sweep loop turns the JsonException into one "ERROR: ..." row in summary.txt and moves to the next plan, so a Query Store sweep over a few hundred plans silently analyzes all but the deep ones. Query Store batch analysis is exactly where the deep plans are. The interesting part is WHY it was missed, because that decides the fix. A shared constant only helps the call sites that remember to reference it, and the options themselves were still duplicated per command — so editing AnalyzeCommand's two sets to add MaxDepth did nothing for the identical pair one file over, and nothing anywhere said they were supposed to match. The options now live on AnalysisJson next to the constant, as IndentedWithoutNulls and CompactWithoutNulls, and both commands point at them. There is no longer a copy to forget. Values are unchanged for analyze: same WriteIndented, same JsonIgnoreCondition.WhenWritingNull, same ceiling. Only querystore's behavior changes, and only from "fails on deep plans" to "does not". The test walks the options instead of trusting the call sites, since trusting them is what went wrong: every static JsonSerializerOptions on a *Command type in PlanViewer.Cli must carry the ceiling. Verified by reverting just the QueryStoreCommand hunk, at which point it fails naming both offenders — "QueryStoreCommand.JsonOptions (MaxDepth 0), QueryStoreCommand.CompactJsonOptions (MaxDepth 0)". A new command that rolls its own now fails here rather than in someone's Query Store sweep. Tested: 82 tests across AnalysisJsonDepthTests and the CLI/analysis classes, 0 failures. dotnet build clean, 0 warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 20, 2026
|
Reviewed. This is a tight, well-scoped fix.
No issues found. |
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
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.
Completes #430. The crash fix in #431 was right; its enumeration of call sites was one file short.
What was missed
#431 made the depth ceiling a shared constant and referenced it from every serializer it found: both Robot Advice entry points, the MCP tool options, and AnalyzeCommand's two option sets.
QueryStoreCommandbuilds its ownJsonOptionsandCompactJsonOptions, identical in shape to AnalyzeCommand's, and neither ever gotMaxDepth.So
planview querystorehas kept failing on any plan deeper than roughly 30 operators for as long asanalyzehas been fine.Why nobody reported it
It fails quietly. The per-plan
try/catchin the sweep loop turns theJsonExceptioninto oneERROR: ...row insummary.txtand moves on to the next plan — so a sweep over a few hundred Query Store plans silently analyzes all but the deep ones. Query Store batch analysis is exactly where the deep plans are.Why the fix isn't just two more
MaxDepth =linesA shared constant only helps the call sites that remember to reference it, and the options were still duplicated per command. Editing AnalyzeCommand's two sets did nothing for the identical pair one file over, and nothing anywhere said they were supposed to match.
The options now live on
AnalysisJsonnext to the constant —IndentedWithoutNullsandCompactWithoutNulls— and both commands point at them. There is no longer a copy to forget.Values are unchanged for
analyze: sameWriteIndented, sameJsonIgnoreCondition.WhenWritingNull, same ceiling. Onlyquerystorebehaviour changes, and only from "fails on deep plans" to "doesn't".Test
It walks the options rather than trusting the call sites, since trusting them is what went wrong: every static
JsonSerializerOptionson a*Commandtype inPlanViewer.Climust carry the ceiling.Verified by reverting just the QueryStoreCommand hunk, at which point it fails naming both offenders:
A new command that rolls its own now fails here rather than in someone's Query Store sweep.
82 tests across
AnalysisJsonDepthTestsand the CLI/analysis classes, 0 failures.dotnet buildclean.🤖 Generated with Claude Code