Skip to content

Commit 754b184

Browse files
Merge pull request #267 from erikdarlingdata/dev
Release v1.7.7 — D3 content strip + legacy marker + drop Rule 21
2 parents 7009393 + aabbaa2 commit 754b184

13 files changed

Lines changed: 145 additions & 339 deletions

File tree

src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
using System;
1+
using System;
22
using System.Collections.Generic;
33
using System.Collections.ObjectModel;
44
using System.ComponentModel;
@@ -1739,9 +1739,10 @@ private void ShowPropertiesPanel(PlanNode node)
17391739
var warnColor = w.Severity == PlanWarningSeverity.Critical ? "#E57373"
17401740
: w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF";
17411741
var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) };
1742+
var legacyTag = w.IsLegacy ? " [legacy]" : "";
17421743
var planWarnHeader = w.MaxBenefitPercent.HasValue
1743-
? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
1744-
: $"\u26A0 {w.WarningType}";
1744+
? $"\u26A0 {w.WarningType}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
1745+
: $"\u26A0 {w.WarningType}{legacyTag}";
17451746
warnPanel.Children.Add(new TextBlock
17461747
{
17471748
Text = planWarnHeader,
@@ -1821,9 +1822,10 @@ private void ShowPropertiesPanel(PlanNode node)
18211822
var warnColor = w.Severity == PlanWarningSeverity.Critical ? "#E57373"
18221823
: w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF";
18231824
var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) };
1825+
var nodeLegacyTag = w.IsLegacy ? " [legacy]" : "";
18241826
var nodeWarnHeader = w.MaxBenefitPercent.HasValue
1825-
? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
1826-
: $"\u26A0 {w.WarningType}";
1827+
? $"\u26A0 {w.WarningType}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
1828+
: $"\u26A0 {w.WarningType}{nodeLegacyTag}";
18271829
warnPanel.Children.Add(new TextBlock
18281830
{
18291831
Text = nodeWarnHeader,

src/PlanViewer.App/PlanViewer.App.csproj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
<ApplicationManifest>app.manifest</ApplicationManifest>
77
<ApplicationIcon>EDD.ico</ApplicationIcon>
88
<AvaloniaUseCompiledBindingsByDefault>true</AvaloniaUseCompiledBindingsByDefault>
9-
<Version>1.7.6</Version>
9+
<Version>1.7.7</Version>
1010
<Authors>Erik Darling</Authors>
1111
<Company>Darling Data LLC</Company>
1212
<Product>Performance Studio</Product>

src/PlanViewer.Core/Models/PlanModels.cs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,14 @@ public class PlanWarning
385385
/// Short actionable fix suggestion (e.g., "Add INCLUDE (columns) to index").
386386
/// </summary>
387387
public string? ActionableFix { get; set; }
388+
389+
/// <summary>
390+
/// True for rules that pre-date the benefit-scoring framework (#215) and haven't
391+
/// been folded into A/B/C/D categorization yet. Joe wanted these visibly marked so
392+
/// reviewers know which items to hold to a higher bar vs which are known-legacy.
393+
/// Renderers show a "legacy" badge when true.
394+
/// </summary>
395+
public bool IsLegacy { get; set; }
388396
}
389397

390398
public enum PlanWarningSeverity { Info, Warning, Critical }

src/PlanViewer.Core/Output/AnalysisResult.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,13 @@ public class WarningResult
226226

227227
[JsonPropertyName("actionable_fix")]
228228
public string? ActionableFix { get; set; }
229+
230+
/// <summary>
231+
/// True for rules predating the benefit-scoring framework. Renderers show a
232+
/// "legacy" badge to distinguish from new-framework warnings.
233+
/// </summary>
234+
[JsonPropertyName("is_legacy")]
235+
public bool IsLegacy { get; set; }
229236
}
230237

231238
public class MissingIndexResult

src/PlanViewer.Core/Output/HtmlExporter.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,7 @@ .card h3 {
187187
.warn-type { font-size: 0.75rem; font-weight: 600; }
188188
.warn-benefit { font-size: 0.7rem; font-weight: 600; color: var(--text-muted); padding: 0.05rem 0.3rem; border-radius: 3px; background: rgba(0,0,0,0.04); }
189189
.warn-msg { font-size: 0.8rem; color: var(--text); flex-basis: 100%; }
190+
.warn-legacy { font-size: 0.65rem; font-weight: 600; color: var(--text-muted); padding: 0.05rem 0.3rem; border-radius: 3px; background: rgba(0,0,0,0.08); text-transform: uppercase; letter-spacing: 0.05em; }
190191
.warn-fix { font-size: 0.75rem; color: var(--text-secondary); font-style: italic; flex-basis: 100%; border-left: 2px solid var(--border); padding-left: 0.5rem; margin-top: 0.15rem; }
191192
192193
/* Query text */
@@ -460,6 +461,8 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt)
460461
if (w.Operator != null)
461462
sb.AppendLine($"<span class=\"warn-op\">{Encode(w.Operator)}</span>");
462463
sb.AppendLine($"<span class=\"warn-type\">{Encode(w.Type)}</span>");
464+
if (w.IsLegacy)
465+
sb.AppendLine("<span class=\"warn-legacy\" title=\"Legacy rule — predates the benefit-scoring framework\">legacy</span>");
463466
if (w.MaxBenefitPercent.HasValue)
464467
sb.AppendLine($"<span class=\"warn-benefit\">up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit</span>");
465468
sb.AppendLine($"<span class=\"warn-msg\">{Encode(w.Message)}</span>");

src/PlanViewer.Core/Output/ResultMapper.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,8 @@ private static StatementResult MapStatement(PlanStatement stmt)
180180
Severity = w.Severity.ToString(),
181181
Message = w.Message,
182182
MaxBenefitPercent = w.MaxBenefitPercent,
183-
ActionableFix = w.ActionableFix
183+
ActionableFix = w.ActionableFix,
184+
IsLegacy = w.IsLegacy
184185
});
185186
}
186187

@@ -283,7 +284,8 @@ private static OperatorResult MapNode(PlanNode node)
283284
Operator = FormatOperatorLabel(node),
284285
NodeId = node.NodeId,
285286
MaxBenefitPercent = w.MaxBenefitPercent,
286-
ActionableFix = w.ActionableFix
287+
ActionableFix = w.ActionableFix,
288+
IsLegacy = w.IsLegacy
287289
});
288290
}
289291

src/PlanViewer.Core/Output/TextFormatter.cs

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,8 @@ public static void WriteText(AnalysisResult result, TextWriter writer)
171171
var benefitTag = w.MaxBenefitPercent.HasValue
172172
? $" (up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit)"
173173
: "";
174-
writer.WriteLine($" [{w.Severity}] {w.Type}{benefitTag}: {EscapeNewlines(w.Message)}");
174+
var legacyTag = w.IsLegacy ? " [legacy]" : "";
175+
writer.WriteLine($" [{w.Severity}] {w.Type}{legacyTag}{benefitTag}: {EscapeNewlines(w.Message)}");
175176
if (!string.IsNullOrEmpty(w.ActionableFix))
176177
writer.WriteLine($" Fix: {EscapeNewlines(w.ActionableFix)}");
177178
}
@@ -298,7 +299,7 @@ private static void WriteGroupedOperatorWarnings(List<WarningResult> warnings, T
298299

299300
// Split each message into "data | explanation" at the last sentence boundary
300301
// that starts with "The " (the harm assessment). Group by shared explanation.
301-
var entries = new List<(string Severity, string Operator, string Data, string? Explanation, double? Benefit)>();
302+
var entries = new List<(string Severity, string Operator, string Data, string? Explanation, double? Benefit, bool IsLegacy)>();
302303
foreach (var w in sorted)
303304
{
304305
var msg = w.Message;
@@ -317,7 +318,7 @@ private static void WriteGroupedOperatorWarnings(List<WarningResult> warnings, T
317318
data = msg;
318319
}
319320

320-
entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent));
321+
entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent, w.IsLegacy));
321322
}
322323

323324
// Group entries that share the same severity, type, and explanation
@@ -334,8 +335,11 @@ private static void WriteGroupedOperatorWarnings(List<WarningResult> warnings, T
334335
// Multiple operators with the same explanation — list compactly
335336
foreach (var item in items)
336337
{
337-
var benefitTag = item.Benefit.HasValue ? $" (up to {item.Benefit:N0}% benefit)" : "";
338-
writer.WriteLine($" [{item.Severity}] {item.Operator}{benefitTag}: {EscapeNewlines(item.Data)}");
338+
var legacyTag = item.IsLegacy ? " [legacy]" : "";
339+
var benefitTag = item.Benefit.HasValue
340+
? $" (up to {(item.Benefit.Value >= 100 ? item.Benefit.Value.ToString("N0") : item.Benefit.Value.ToString("N1"))}% benefit)"
341+
: "";
342+
writer.WriteLine($" [{item.Severity}] {item.Operator}{legacyTag}{benefitTag}: {EscapeNewlines(item.Data)}");
339343
}
340344
writer.WriteLine($" -> {group.Key.Item2}");
341345
}
@@ -345,8 +349,11 @@ private static void WriteGroupedOperatorWarnings(List<WarningResult> warnings, T
345349
foreach (var item in items)
346350
{
347351
var full = item.Explanation != null ? $"{item.Data}. {item.Explanation}" : item.Data;
348-
var benefitTag = item.Benefit.HasValue ? $" (up to {item.Benefit:N0}% benefit)" : "";
349-
writer.WriteLine($" [{item.Severity}] {item.Operator}{benefitTag}: {EscapeNewlines(full)}");
352+
var legacyTag = item.IsLegacy ? " [legacy]" : "";
353+
var benefitTag = item.Benefit.HasValue
354+
? $" (up to {(item.Benefit.Value >= 100 ? item.Benefit.Value.ToString("N0") : item.Benefit.Value.ToString("N1"))}% benefit)"
355+
: "";
356+
writer.WriteLine($" [{item.Severity}] {item.Operator}{legacyTag}{benefitTag}: {EscapeNewlines(full)}");
350357
}
351358
}
352359
}

src/PlanViewer.Core/Services/BenefitScorer.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,9 @@ private static void EmitWaitStatWarnings(PlanStatement stmt)
6565
double? benefitPct = benefitByType.TryGetValue(wait.WaitType, out var b) ? b : null;
6666

6767
var msg = new System.Text.StringBuilder();
68-
msg.Append(wait.WaitType).Append(": ").Append(entry.Description);
68+
msg.Append(wait.WaitType);
69+
if (!string.IsNullOrEmpty(entry.Description))
70+
msg.Append(": ").Append(entry.Description);
6971
msg.Append(" Observed ").Append(wait.WaitTimeMs.ToString("N0")).Append(" ms");
7072
if (wait.WaitCount > 0)
7173
msg.Append(" across ").Append(wait.WaitCount.ToString("N0")).Append(" wait").Append(wait.WaitCount == 1 ? "" : "s");
@@ -92,7 +94,7 @@ private static void EmitWaitStatWarnings(PlanStatement stmt)
9294
Message = msg.ToString(),
9395
Severity = severity,
9496
MaxBenefitPercent = benefitPct,
95-
ActionableFix = entry.HowToFix
97+
ActionableFix = string.IsNullOrEmpty(entry.HowToFix) ? null : entry.HowToFix
9698
});
9799
}
98100
}

src/PlanViewer.Core/Services/PlanAnalyzer.cs

Lines changed: 59 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,6 @@ public static class PlanAnalyzer
2424
@"\bCASE\s+(WHEN\b|$)",
2525
RegexOptions.IgnoreCase | RegexOptions.Compiled);
2626

27-
// Matches CTE definitions: WITH name AS ( or , name AS (
28-
private static readonly Regex CteDefinitionRegex = new(
29-
@"(?:\bWITH\s+|\,\s*)(\w+)\s+AS\s*\(",
30-
RegexOptions.IgnoreCase | RegexOptions.Compiled);
31-
3227
public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null)
3328
{
3429
var cfg = config ?? AnalyzerConfig.Default;
@@ -40,6 +35,8 @@ public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null)
4035

4136
if (stmt.RootNode != null)
4237
AnalyzeNodeTree(stmt.RootNode, stmt, cfg);
38+
39+
MarkLegacyWarnings(stmt);
4340
}
4441
}
4542

@@ -48,6 +45,59 @@ public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null)
4845
ApplySeverityOverrides(plan, cfg);
4946
}
5047

48+
/// <summary>
49+
/// Rule types that predate the benefit-scoring framework (#215) and haven't
50+
/// been folded into A/B/C/D categorization yet. Tagged so reviewers can hold
51+
/// new-framework items to a higher bar vs known-legacy items that will be
52+
/// reworked later. Remove entries from this set as rules migrate.
53+
/// </summary>
54+
private static readonly HashSet<string> LegacyWarningTypes = new(StringComparer.OrdinalIgnoreCase)
55+
{
56+
"Excessive Memory Grant",
57+
"Large Memory Grant",
58+
"Compile Memory Exceeded",
59+
"Local Variables",
60+
"Optimize For Unknown",
61+
"Low Impact Index",
62+
"Wide Index Suggestion",
63+
"Duplicate Index Suggestions",
64+
"Table Variable",
65+
"Scalar UDF",
66+
"Parallel Skew",
67+
"Estimated Plan CE Guess",
68+
"Data Type Mismatch",
69+
"Lazy Spool Ineffective",
70+
"Join OR Clause",
71+
"Many-to-Many Merge Join",
72+
"Table-Valued Function",
73+
"Top Above Scan",
74+
"Row Goal",
75+
"NOT IN with Nullable Column",
76+
"Implicit Conversion",
77+
};
78+
79+
private static void MarkLegacyWarnings(PlanStatement stmt)
80+
{
81+
foreach (var w in stmt.PlanWarnings)
82+
{
83+
if (LegacyWarningTypes.Contains(w.WarningType))
84+
w.IsLegacy = true;
85+
}
86+
if (stmt.RootNode != null)
87+
MarkLegacyWarningsOnTree(stmt.RootNode);
88+
}
89+
90+
private static void MarkLegacyWarningsOnTree(PlanNode node)
91+
{
92+
foreach (var w in node.Warnings)
93+
{
94+
if (LegacyWarningTypes.Contains(w.WarningType))
95+
w.IsLegacy = true;
96+
}
97+
foreach (var child in node.Children)
98+
MarkLegacyWarningsOnTree(child);
99+
}
100+
51101
// Rule number → WarningType mapping for severity overrides
52102
private static readonly Dictionary<int, string> RuleWarningTypes = new()
53103
{
@@ -58,7 +108,7 @@ public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null)
58108
[13] = "Data Type Mismatch", [14] = "Lazy Spool Ineffective", [15] = "Join OR Clause",
59109
[16] = "Nested Loops High Executions", [17] = "Many-to-Many Merge Join",
60110
[18] = "Compile Memory Exceeded", [19] = "High Compile CPU", [20] = "Local Variables",
61-
[21] = "CTE Multiple References", [22] = "Table Variable", [23] = "Table-Valued Function",
111+
[22] = "Table Variable", [23] = "Table-Valued Function",
62112
[24] = "Top Above Scan", [25] = "Ineffective Parallelism", [26] = "Row Goal",
63113
[27] = "Optimize For Unknown", [28] = "NOT IN with Nullable Column",
64114
[29] = "Implicit Conversion", [30] = "Wide Index Suggestion",
@@ -367,11 +417,9 @@ private static void AnalyzeStatement(PlanStatement stmt, AnalyzerConfig cfg)
367417
}
368418
}
369419

370-
// Rule 21: CTE referenced multiple times
371-
if (!cfg.IsRuleDisabled(21) && !string.IsNullOrEmpty(stmt.StatementText))
372-
{
373-
DetectMultiReferenceCte(stmt);
374-
}
420+
// Rule 21 (CTE referenced multiple times) removed per Joe's #215 feedback:
421+
// for actual plans, SQL Server runtime stats show exactly where time was
422+
// spent, so a statement-text-pattern warning about CTE reuse is guessing.
375423

376424
// Rule 27: OPTIMIZE FOR UNKNOWN in statement text
377425
if (!cfg.IsRuleDisabled(27) && !string.IsNullOrEmpty(stmt.StatementText) &&
@@ -1445,41 +1493,6 @@ private static bool IsFunctionOnColumnSide(string predicate, Match funcMatch)
14451493
return Regex.IsMatch(side, @"\[[^\]@]+\]\.\[");
14461494
}
14471495

1448-
/// <summary>
1449-
/// Detects CTEs that are referenced more than once in the statement text.
1450-
/// Each reference re-executes the CTE since SQL Server does not materialize them.
1451-
/// </summary>
1452-
private static void DetectMultiReferenceCte(PlanStatement stmt)
1453-
{
1454-
var text = stmt.StatementText;
1455-
var cteMatches = CteDefinitionRegex.Matches(text);
1456-
if (cteMatches.Count == 0)
1457-
return;
1458-
1459-
foreach (Match match in cteMatches)
1460-
{
1461-
var cteName = match.Groups[1].Value;
1462-
if (string.IsNullOrEmpty(cteName))
1463-
continue;
1464-
1465-
// Count references as FROM/JOIN targets after the CTE definition
1466-
var refPattern = new Regex(
1467-
$@"\b(FROM|JOIN)\s+{Regex.Escape(cteName)}\b",
1468-
RegexOptions.IgnoreCase);
1469-
var refCount = refPattern.Matches(text).Count;
1470-
1471-
if (refCount > 1)
1472-
{
1473-
stmt.PlanWarnings.Add(new PlanWarning
1474-
{
1475-
WarningType = "CTE Multiple References",
1476-
Message = $"CTE \"{cteName}\" is referenced {refCount} times. SQL Server re-executes the entire CTE each time — it does not materialize the results. Materialize into a #temp table instead.",
1477-
Severity = PlanWarningSeverity.Warning
1478-
});
1479-
}
1480-
}
1481-
}
1482-
14831496
/// <summary>
14841497
/// Verifies the OR expansion chain walking up from a Concatenation node:
14851498
/// Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation

0 commit comments

Comments
 (0)