Skip to content

Commit aabbaa2

Browse files
erikdarlingdataclaude
erikdarlingdata
and
claude
authored
Strip AI-drafted wait stat content + legacy marker + drop Rule 21 (#215 D3) (#266)
* Strip AI-drafted wait-stat descriptions + legacy marker + remove Rule 21 (#215 D3, framework marker, Joe's categorization refinements) D3: WaitStatsKnowledge Description / HowToFix text was AI-drafted without expert review. Joe flagged the SOS_SCHEDULER_YIELD fix text as misleading and asked that creative copy come from SQL Server experts, not AI. Stripped all Description and HowToFix content; kept the file structure and the ShowEffectiveLatency flag (structural, not creative). Entries are still emitted but the warning message now shows only the wait name, observed ms and wait count, and — for PAGEIOLATCH_* — effective latency. Erik / Joe fill in content over time; the pipeline is ready to render it when they do. BenefitScorer.EmitWaitStatWarnings now omits the description separator and sets ActionableFix to null when the entry has no HowToFix, so the renderers don't print empty "Fix:" prefixes. Framework marker (option a, flipped): PlanWarning + WarningResult grow an IsLegacy bool. Joe's preference: tag old rules so new ones don't need re-tagging when the rest get migrated. A LegacyWarningTypes set in PlanAnalyzer lists rules that haven't been folded into the benefit-scoring framework yet; MarkLegacyWarnings post-pass sets IsLegacy = true on matches. Renderers (web strip, HTML export, Avalonia plan + node warnings, TextFormatter statement + grouped operator paths) all show a "legacy" badge or " [legacy]" tag when set. Rule 21 (CTE referenced multiple times) removed per Joe's feedback — actual plans already show where time goes; statement-text pattern matching on CTE references is guessing. Removed the rule, its regex, the rule map entry, and DetectMultiReferenceCte helper. TextFormatter operator-warning benefit % now also uses the N1-except-100 formatter so grouped operator output is consistent with the rest of the UI. Version bump 1.7.6 -> 1.7.7. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Restore CRLF line endings in PlanViewerControl.axaml.cs Prior commit's Python patch script wrote LF line endings. This file is checked in with CRLF; restoring. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop Rule 21 test — rule was removed in prior commit Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent e05bc69 commit aabbaa2

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)