Plan viewer: inner-side Nested Loops edges show a huge misestimate when the node label says the estimate was close #4627

Activity

1. erikdarlingdata

   added

   in-progressActively being worked by a local session or its agents (PR open or in flight)Actively being worked by a local session or its agents (PR open or in flight)

   on Sep 28, 2026
2. erikdarlingdata

   added a commit that references this issue on Sep 28, 2026

   Plan viewer: fix edge/label misestimate mismatch, zero-grant text, Li…

   ...

   5e0e737
3. erikdarlingdata commented on Sep 28, 2026

   

   erikdarlingdata

   on Sep 28, 2026

   OwnerAuthor

   More actions

   Claude posting for Erik Darling

   Fixed by #4632, merged to dev as 5e0e737. Checked in an off-screen harness on the merged code. On udf_plan, the edges went from 6 grey and 5 FluoRed to 10 grey and 1 light orange. That matches the nodes the PR pins (7, 8, 10 and 12 no longer FluoRed).
4. erikdarlingdata

   closed this as completedon Sep 28, 2026
5. erikdarlingdata

   removed

   in-progressActively being worked by a local session or its agents (PR open or in flight)Actively being worked by a local session or its agents (PR open or in flight)

   on Sep 28, 2026
6. erikdarlingdata commented on Sep 29, 2026

   

   erikdarlingdata

   on Sep 29, 2026

   OwnerAuthor

   More actions

   Claude posting for Erik Darling. Lane W handoff for the #4627 follow-up. No code written, no PR opened, nothing pushed.

   Status

   The lane hit the context watchdog (300k) during orientation, before the first edit. The local branch feature/4627-row-estimate-nl-parallel exists in the worktree .claude/worktrees/agent-a805bd35f0256a74f at 86e9932 (the #4688 head), with no changes. Re-queue as one lane. Everything below is verified against the sources unless marked "not verified".

   Corrections to the brief (found in PS #597 and in PM)

   1. Rule 16 (Nested Loops, PlanAnalyzer.cs ~1333-1350) is NOT fine. PS Fix High Impact query preview — full text in tooltip, ellipsis in row #597 changed exactly this block. PM divides outerChild.ActualRows by outerChild.ActualExecutions, which is a thread count in a parallel zone, so the outer-side ratio is understated by DOP and the "actual" figure printed is wrong. Port PS's block (RowEstimateHelper.GetRowAccuracyRatio(outerChild); the printed actual is the per-execution figure only when IsInnerSideOfNestedLoops(outerChild) && ActualExecutions > 0, else the total).
   2. Rule 26 (~1499-1506) has the same defect (executions = ActualExecutions > 0 ? ... : 1, then rowGoalWorked = actualPerExec <= EstimateRows). PS: rowGoalWorked = RowEstimateHelper.GetRowAccuracyRatio(node) <= 1.0.
   3. Rule 5 (~849-895): PS's shape is var isInnerSide = IsInnerSideOfNestedLoops(node); var ratio = GetRowAccuracyRatio(node); and the "(N rows x M executions)" text is printed only when isInnerSide && ActualExecutions > 1. PM already has PS's ActualExecutions > 0 and !node.Lookup guards, so on the Key Lookup rule 5 gives NO finding in both apps (PS PlanAnalyzer.Node.cs:162, !node.Lookup).
   4. Rule 32 (~1143) stays as is. PS Fix High Impact query preview — full text in tooltip, ellipsis in row #597 did not touch it (it compares the raw total to the raw estimate).
   5. The DOP 8 rule-5 pin cannot be "actual equals estimate". The old ratio there is 1/8 = 0.125, above the 0.1 gate, so old code never fired: no RED. Use PS's numbers: join node, LogicalOp = "Inner Join", under a Gather Streams parent, EstimateRows 2983.02, ActualRows 609, ActualExecutions 8. Old code: "39x overestimated". New: silent. The direction is over, not under. For a colour RED at "accurate operator" use DOP 11 or more (old ratio 1/11 = 0.0909 is under 0.1, so the label goes orange and the edge goes Blue). At DOP 8 the discriminating assertion is the label string: "100%" now, the old code printed 12.5% through F0 (I expect "13%"; confirm when the RED runs).

   Sites to change (PM)

   • New PerformanceMonitor.PlanAnalysis/RowEstimateHelper.cs, a faithful port (same name as PS). PS source, read-only: git -C C:\GitHub\PerformanceStudio show origin/dev:src/PlanViewer.Core/Services/RowEstimateHelper.cs. Members: IsInnerSideOfNestedLoops(PlanNode) (walk EVERY ancestor: PhysicalOp == "Nested Loops" && Children.Count > 1 && Children[1] == child), GetExpectedRows (ActualExecutions > 0 && inner ? EstimateRows * ActualExecutions : EstimateRows), GetRowAccuracyRatio (expected > 0 ? ActualRows / expected : (ActualRows > 0 ? double.MaxValue : 1.0)). PM's parser sets Parent (ShowPlanParser.cs:468/575/1613), and there is no == overload on PlanNode.
   • PlanRowAccuracy.cs: delete ActualRowsPerExecution and Ratio; replace FormatActualOfEstimate with the totals formatter below. Its only callers are Rendering.cs:274-281, PlanEdgeColour.cs:73-74, Viewer4627Tests and Viewer4684RowLabelTests.
   • PlanEdgeColour.ForChild(bool hasActualStats, double actualRows, long actualExecutions, double estimateRows, double limit): change to take expectedRows in place of (actualExecutions, estimateRows) (or take the PlanNode). Rewrite its doc comment, which still says it diverges from PS until PS#594. Callers: Rendering.cs:400, Minimap.cs:193 (the minimap uses its own GetLinkColorBrush(child, clampedLimit) overload), Viewer4579Tests (17 calls, all pass 1 executions, so dropping that argument is mechanical), Viewer4627Tests lines 90 and 113.
   • PerformanceMonitor.Ui/PlanViewerControl.Rendering.cs ~269-283: label text from the formatter with RowEstimateHelper.GetExpectedRows(node); brush ratio from GetRowAccuracyRatio(node) (thresholds 0.1 / 10.0 stay).
   • Left alone on purpose, state in the PR: the edge tooltip (Rendering.cs ~447-455, shows raw SSMS-style figures, no ratio), Properties/Tooltips rows, McpPlanAnalysisFormatter (emits raw estimated_rows / actual_rows; PS's expected_rows JSON/HTML addition has no PM counterpart and adding a field touches the MCP payload census), Rule 32. Dashboard copy: my grep of deprecated/ for ActualRowsPerExec|accuracyRatio|ActualExecutions found nothing, but re-check EstimateRows there before writing "no Dashboard copy" (not verified).

   Formatter (the coordinator's rule, plus the edge-case calls I made)

   FormatActualOfExpected(double actualRows, double expectedRows, IFormatProvider? provider = null):

   • showPercent = expectedRows > 0; pct = (actualRows / expectedRows * 100).ToString("F0", provider) from the unrounded values.
   • Loop d from 0 to 4. Whole numbers (Math.Floor(v) == v) always print N0; non-whole print "N" + d. Stop at the first d where the percent from the PRINTED numbers equals pct, or at d == 4.
   • Parse the printed strings back with double.TryParse(text, NumberStyles.Number, provider, ...). The old code used NumberStyles.Float, which rejects "2,983" and de-DE "1.234,5". Never use G format (it prints "5E-05").
   • Division by a printed 0 gives NaN or Infinity, whose F0 string differs from pct, so it counts as a disagreement and adds decimals (0 actual of 0.4 expected prints "0 of 0.4 (0%)"). State this in the PR so PS matches.
   • Consequence to state: an estimate under 0.00005 prints "0.0000" after the 4-decimal stop.
   • Computed, not trusted: key_lookup_plan Node 4 (Clustered Index Seek with Lookup, est 0.00964372, 1 actual row over 117 executions, serial plan) gives expected 1.12832 and pct 88.63, which prints 89. N0 "1" gives 100, N1 "1.1" gives 91, N2 "1.13" gives 88, N3 "1.128" gives 89. Label: "1 of 1.128 (89%)". The old label was "0.0085 of 0.0096 (89%)".

   Pins (each RED against the old code)

   1. Node 4 above: exact label, invariant culture.
   2. DOP 8 not on an NL inner side, actual equal to the estimate: label "100%" (old "13%"); DOP 11+ variant for the label brush and edge colour. Build it as XML with 8 RunTimeCountersPerThread rows, one execution each, so the parser sums ActualExecutions to 8.
   3. A node under two NL inner sides: hand-built or XML, for example Est 0.5, 1000 executions, 400 rows gives "400 of 500 (80%)", old "0.4 of 0.5 (80%)". udf_plan.sqlplan has no such node (its nodes 7 to 12 are one level deep; keep them for Viewer4627Tests, where ratios are numerically unchanged).
   4. Formatter rule: whole number unchanged ("609 of 2,983 (20%)"); contradiction resolved with fewest decimals; the 4-decimal cap (for example actual 1, expected 0.00012345: compute the pct, do not trust me); tiny estimate 0.00005 prints fixed-point (never "E-05"); de-DE ("1 of 1,128 (89%)").
   5. Analyzer: rule 5 DOP 8 (numbers in correction 5); rule 5 key lookup silent; rule 5 NL inner non-lookup still fires (PS Rule05_InnerSideNodeWithARealMismatch: Est 1, 50,000 actual over 1,000 executions gives "50x underestimated ... (50 rows x 1,000 executions)", identical old and new, so no RED); rule 16 (PS gate test: outer est 100, actual 100,000, 8 executions, inner Key Lookup 200,000 executions gives "actual 100,000 (1000x underestimate)", old "12,500 (125x)"); rule 26 (est 100, without-row-goal 1000, actual 500, 8 executions: fires now, old code swallowed it).

   • Reuse PS's fixture eager_index_spool_plan.sqlplan (DOP 8; Node 1 not inner: "609 of 2,983 (20%)"; Nodes 4 and 5 inner: "609 of 613 (99%)"; Node 6 inner, one execution). I did not locate it in the PS tree (find it with git ls-tree -r origin/dev --name-only | grep eager_index); copy it to Darling/Darling.Tests/Fixtures/OriginPlans/ (csproj glob Fixtures\OriginPlans\*.sqlplan picks it up).
   • Rewrite: all of Viewer4684RowLabelTests (labels go totals-form; the 2000-iteration sweep must tolerate the 4-decimal stop; the source-scan pin string changes), Viewer4627Tests (API only). Name new Darling.Tests classes Viewer* so the Viewer* run covers them, and run PlanSync* too (PlanSync4535NodeRulesATests has a rule-5 inner-side test that must stay green).
   • A live label/edge/minimap pin in Lite.Tests is possible by reflection on the private CreateNodeVisual(PlanNode, int), GetLinkColorBrush(PlanNode) and GetLinkColorBrush(PlanNode, double), with Lite's LightTheme merged into a host Border (pattern: Lite.Tests/PlanViewer4632FallbackTests.cs). Not verified that CreateNodeVisual runs headless (tooltip, context menu, icon); fall back to source pins if not.
   • RED proof: commit first, then git checkout 86e99328c -- <product files>, run the new classes, restore with git checkout HEAD -- <files>. Push a checkpoint commit after the helper, edge, minimap, analyzer and their pins, with the label call still on the old formatter; then the label commit.

   CHANGELOG (put in the PR body; never edit CHANGELOG.md)

   Two unshipped entries are now wrong and need replacing: #4632's "come from the same per-execution ratio" and #4688's row-line entry ("now reads 0.0085 of 0.0096 (89%)"). Suggested replacement for the #4627 entry:

   SECTION: Fixed
   ENTRY:

   • Plan viewer row estimates compare like with like: an accurate parallel operator no longer reads as a misestimate, and the row line shows total rows against the expected total ([#PR]) - the node's "X of Y" label, its edge colour, the minimap and the Row Estimate Mismatch, Nested Loops and Row Goal findings divided actual rows by the operator's execution count for every node, but in a parallel zone that count is the thread count, so an accurate operator at DOP 8 read 12.5% and from DOP 11 turned red. The estimate is now multiplied by executions only on the inner side of a Nested Loops join. A Key Lookup that ran 117 times for 1 row reads "1 of 1.128 (89%)".
     REF:
     [#PR]: https://github.com/erikdarlingdata/PerformanceMonitor/pull/PR

   Open the PR as a draft against dev, stacked on #4688 (feature/4684-plus-tab-uia-and-row-label); if #4688 merges first, merge origin/dev. Write the formatter rule into the PR body word for word as the brief gives it, plus the two edge-case calls above.
7. erikdarlingdata

   added 5 commits that reference this issue on Sep 29, 2026

   Plan viewer: the node row label prints the totals against the expecte…

   ...

   489bf70

   Plan viewer: the edge colour and minimap take expected rows from RowE…

   ...

   2f1c5a2

   Plan viewer: delete the per-execution row members nothing calls any m…

   ...

   6c16dc7

   Plan viewer: plain wording in five comments and one test name, and pi…

   ...

   155350a

   Plan viewer and analyzer: compare row estimates the way PerformanceSt…

   ...

   8c2b870