Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.ComponentModel;
Expand Down Expand Up @@ -1739,9 +1739,10 @@ private void ShowPropertiesPanel(PlanNode node)
var warnColor = w.Severity == PlanWarningSeverity.Critical ? "#E57373"
: w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF";
var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) };
var legacyTag = w.IsLegacy ? " [legacy]" : "";
var planWarnHeader = w.MaxBenefitPercent.HasValue
? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
: $"\u26A0 {w.WarningType}";
? $"\u26A0 {w.WarningType}{legacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
: $"\u26A0 {w.WarningType}{legacyTag}";
warnPanel.Children.Add(new TextBlock
{
Text = planWarnHeader,
Expand Down Expand Up @@ -1821,9 +1822,10 @@ private void ShowPropertiesPanel(PlanNode node)
var warnColor = w.Severity == PlanWarningSeverity.Critical ? "#E57373"
: w.Severity == PlanWarningSeverity.Warning ? "#FFB347" : "#6BB5FF";
var warnPanel = new StackPanel { Margin = new Thickness(10, 2, 10, 2) };
var nodeLegacyTag = w.IsLegacy ? " [legacy]" : "";
var nodeWarnHeader = w.MaxBenefitPercent.HasValue
? $"\u26A0 {w.WarningType} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
: $"\u26A0 {w.WarningType}";
? $"\u26A0 {w.WarningType}{nodeLegacyTag} \u2014 up to {FormatBenefitPercent(w.MaxBenefitPercent.Value)}% benefit"
: $"\u26A0 {w.WarningType}{nodeLegacyTag}";
warnPanel.Children.Add(new TextBlock
{
Text = nodeWarnHeader,
Expand Down
2 changes: 1 addition & 1 deletion src/PlanViewer.App/PlanViewer.App.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
<ApplicationManifest>app.manifest</ApplicationManifest>
<ApplicationIcon>EDD.ico</ApplicationIcon>
<AvaloniaUseCompiledBindingsByDefault>true</AvaloniaUseCompiledBindingsByDefault>
<Version>1.7.6</Version>
<Version>1.7.7</Version>
<Authors>Erik Darling</Authors>
<Company>Darling Data LLC</Company>
<Product>Performance Studio</Product>
Expand Down
8 changes: 8 additions & 0 deletions src/PlanViewer.Core/Models/PlanModels.cs
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,14 @@ public class PlanWarning
/// Short actionable fix suggestion (e.g., "Add INCLUDE (columns) to index").
/// </summary>
public string? ActionableFix { get; set; }

/// <summary>
/// True for rules that pre-date the benefit-scoring framework (#215) and haven't
/// been folded into A/B/C/D categorization yet. Joe wanted these visibly marked so
/// reviewers know which items to hold to a higher bar vs which are known-legacy.
/// Renderers show a "legacy" badge when true.
/// </summary>
public bool IsLegacy { get; set; }
}

public enum PlanWarningSeverity { Info, Warning, Critical }
Expand Down
7 changes: 7 additions & 0 deletions src/PlanViewer.Core/Output/AnalysisResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,13 @@ public class WarningResult

[JsonPropertyName("actionable_fix")]
public string? ActionableFix { get; set; }

/// <summary>
/// True for rules predating the benefit-scoring framework. Renderers show a
/// "legacy" badge to distinguish from new-framework warnings.
/// </summary>
[JsonPropertyName("is_legacy")]
public bool IsLegacy { get; set; }
}

public class MissingIndexResult
Expand Down
3 changes: 3 additions & 0 deletions src/PlanViewer.Core/Output/HtmlExporter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ .card h3 {
.warn-type { font-size: 0.75rem; font-weight: 600; }
.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); }
.warn-msg { font-size: 0.8rem; color: var(--text); flex-basis: 100%; }
.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; }
.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; }

/* Query text */
Expand Down Expand Up @@ -460,6 +461,8 @@ private static void WriteWarnings(StringBuilder sb, StatementResult stmt)
if (w.Operator != null)
sb.AppendLine($"<span class=\"warn-op\">{Encode(w.Operator)}</span>");
sb.AppendLine($"<span class=\"warn-type\">{Encode(w.Type)}</span>");
if (w.IsLegacy)
sb.AppendLine("<span class=\"warn-legacy\" title=\"Legacy rule — predates the benefit-scoring framework\">legacy</span>");
if (w.MaxBenefitPercent.HasValue)
sb.AppendLine($"<span class=\"warn-benefit\">up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit</span>");
sb.AppendLine($"<span class=\"warn-msg\">{Encode(w.Message)}</span>");
Expand Down
6 changes: 4 additions & 2 deletions src/PlanViewer.Core/Output/ResultMapper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ private static StatementResult MapStatement(PlanStatement stmt)
Severity = w.Severity.ToString(),
Message = w.Message,
MaxBenefitPercent = w.MaxBenefitPercent,
ActionableFix = w.ActionableFix
ActionableFix = w.ActionableFix,
IsLegacy = w.IsLegacy
});
}

Expand Down Expand Up @@ -283,7 +284,8 @@ private static OperatorResult MapNode(PlanNode node)
Operator = FormatOperatorLabel(node),
NodeId = node.NodeId,
MaxBenefitPercent = w.MaxBenefitPercent,
ActionableFix = w.ActionableFix
ActionableFix = w.ActionableFix,
IsLegacy = w.IsLegacy
});
}

Expand Down
21 changes: 14 additions & 7 deletions src/PlanViewer.Core/Output/TextFormatter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ public static void WriteText(AnalysisResult result, TextWriter writer)
var benefitTag = w.MaxBenefitPercent.HasValue
? $" (up to {(w.MaxBenefitPercent.Value >= 100 ? w.MaxBenefitPercent.Value.ToString("N0") : w.MaxBenefitPercent.Value.ToString("N1"))}% benefit)"
: "";
writer.WriteLine($" [{w.Severity}] {w.Type}{benefitTag}: {EscapeNewlines(w.Message)}");
var legacyTag = w.IsLegacy ? " [legacy]" : "";
writer.WriteLine($" [{w.Severity}] {w.Type}{legacyTag}{benefitTag}: {EscapeNewlines(w.Message)}");
if (!string.IsNullOrEmpty(w.ActionableFix))
writer.WriteLine($" Fix: {EscapeNewlines(w.ActionableFix)}");
}
Expand Down Expand Up @@ -298,7 +299,7 @@ private static void WriteGroupedOperatorWarnings(List<WarningResult> warnings, T

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

entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent));
entries.Add((w.Severity, w.Operator ?? "?", data, explanation, w.MaxBenefitPercent, w.IsLegacy));
}

// Group entries that share the same severity, type, and explanation
Expand All @@ -334,8 +335,11 @@ private static void WriteGroupedOperatorWarnings(List<WarningResult> warnings, T
// Multiple operators with the same explanation — list compactly
foreach (var item in items)
{
var benefitTag = item.Benefit.HasValue ? $" (up to {item.Benefit:N0}% benefit)" : "";
writer.WriteLine($" [{item.Severity}] {item.Operator}{benefitTag}: {EscapeNewlines(item.Data)}");
var legacyTag = item.IsLegacy ? " [legacy]" : "";
var benefitTag = item.Benefit.HasValue
? $" (up to {(item.Benefit.Value >= 100 ? item.Benefit.Value.ToString("N0") : item.Benefit.Value.ToString("N1"))}% benefit)"
: "";
writer.WriteLine($" [{item.Severity}] {item.Operator}{legacyTag}{benefitTag}: {EscapeNewlines(item.Data)}");
}
writer.WriteLine($" -> {group.Key.Item2}");
}
Expand All @@ -345,8 +349,11 @@ private static void WriteGroupedOperatorWarnings(List<WarningResult> warnings, T
foreach (var item in items)
{
var full = item.Explanation != null ? $"{item.Data}. {item.Explanation}" : item.Data;
var benefitTag = item.Benefit.HasValue ? $" (up to {item.Benefit:N0}% benefit)" : "";
writer.WriteLine($" [{item.Severity}] {item.Operator}{benefitTag}: {EscapeNewlines(full)}");
var legacyTag = item.IsLegacy ? " [legacy]" : "";
var benefitTag = item.Benefit.HasValue
? $" (up to {(item.Benefit.Value >= 100 ? item.Benefit.Value.ToString("N0") : item.Benefit.Value.ToString("N1"))}% benefit)"
: "";
writer.WriteLine($" [{item.Severity}] {item.Operator}{legacyTag}{benefitTag}: {EscapeNewlines(full)}");
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions src/PlanViewer.Core/Services/BenefitScorer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ private static void EmitWaitStatWarnings(PlanStatement stmt)
double? benefitPct = benefitByType.TryGetValue(wait.WaitType, out var b) ? b : null;

var msg = new System.Text.StringBuilder();
msg.Append(wait.WaitType).Append(": ").Append(entry.Description);
msg.Append(wait.WaitType);
if (!string.IsNullOrEmpty(entry.Description))
msg.Append(": ").Append(entry.Description);
msg.Append(" Observed ").Append(wait.WaitTimeMs.ToString("N0")).Append(" ms");
if (wait.WaitCount > 0)
msg.Append(" across ").Append(wait.WaitCount.ToString("N0")).Append(" wait").Append(wait.WaitCount == 1 ? "" : "s");
Expand All @@ -92,7 +94,7 @@ private static void EmitWaitStatWarnings(PlanStatement stmt)
Message = msg.ToString(),
Severity = severity,
MaxBenefitPercent = benefitPct,
ActionableFix = entry.HowToFix
ActionableFix = string.IsNullOrEmpty(entry.HowToFix) ? null : entry.HowToFix
});
}
}
Expand Down
105 changes: 59 additions & 46 deletions src/PlanViewer.Core/Services/PlanAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,6 @@
@"\bCASE\s+(WHEN\b|$)",
RegexOptions.IgnoreCase | RegexOptions.Compiled);

// Matches CTE definitions: WITH name AS ( or , name AS (
private static readonly Regex CteDefinitionRegex = new(
@"(?:\bWITH\s+|\,\s*)(\w+)\s+AS\s*\(",
RegexOptions.IgnoreCase | RegexOptions.Compiled);

public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null)
{
var cfg = config ?? AnalyzerConfig.Default;
Expand All @@ -40,6 +35,8 @@

if (stmt.RootNode != null)
AnalyzeNodeTree(stmt.RootNode, stmt, cfg);

MarkLegacyWarnings(stmt);
}
}

Expand All @@ -48,6 +45,59 @@
ApplySeverityOverrides(plan, cfg);
}

/// <summary>
/// Rule types that predate the benefit-scoring framework (#215) and haven't
/// been folded into A/B/C/D categorization yet. Tagged so reviewers can hold
/// new-framework items to a higher bar vs known-legacy items that will be
/// reworked later. Remove entries from this set as rules migrate.
/// </summary>
private static readonly HashSet<string> LegacyWarningTypes = new(StringComparer.OrdinalIgnoreCase)
{
"Excessive Memory Grant",
"Large Memory Grant",
"Compile Memory Exceeded",
"Local Variables",
"Optimize For Unknown",
"Low Impact Index",
"Wide Index Suggestion",
"Duplicate Index Suggestions",
"Table Variable",
"Scalar UDF",
"Parallel Skew",
"Estimated Plan CE Guess",
"Data Type Mismatch",
"Lazy Spool Ineffective",
"Join OR Clause",
"Many-to-Many Merge Join",
"Table-Valued Function",
"Top Above Scan",
"Row Goal",
"NOT IN with Nullable Column",
"Implicit Conversion",
};

private static void MarkLegacyWarnings(PlanStatement stmt)
{
foreach (var w in stmt.PlanWarnings)
{
if (LegacyWarningTypes.Contains(w.WarningType))
w.IsLegacy = true;
}
if (stmt.RootNode != null)
MarkLegacyWarningsOnTree(stmt.RootNode);
}

private static void MarkLegacyWarningsOnTree(PlanNode node)
{
foreach (var w in node.Warnings)
{
if (LegacyWarningTypes.Contains(w.WarningType))
w.IsLegacy = true;
}
foreach (var child in node.Children)
MarkLegacyWarningsOnTree(child);
}

// Rule number → WarningType mapping for severity overrides
private static readonly Dictionary<int, string> RuleWarningTypes = new()
{
Expand All @@ -58,7 +108,7 @@
[13] = "Data Type Mismatch", [14] = "Lazy Spool Ineffective", [15] = "Join OR Clause",
[16] = "Nested Loops High Executions", [17] = "Many-to-Many Merge Join",
[18] = "Compile Memory Exceeded", [19] = "High Compile CPU", [20] = "Local Variables",
[21] = "CTE Multiple References", [22] = "Table Variable", [23] = "Table-Valued Function",
[22] = "Table Variable", [23] = "Table-Valued Function",
[24] = "Top Above Scan", [25] = "Ineffective Parallelism", [26] = "Row Goal",
[27] = "Optimize For Unknown", [28] = "NOT IN with Nullable Column",
[29] = "Implicit Conversion", [30] = "Wide Index Suggestion",
Expand Down Expand Up @@ -353,7 +403,7 @@

if (unsnifffedParams.Count > 0)
{
var hasRecompile = stmt.StatementText.Contains("RECOMPILE", StringComparison.OrdinalIgnoreCase);

Check warning on line 406 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test

Dereference of a possibly null reference.

Check warning on line 406 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test

Dereference of a possibly null reference.

Check warning on line 406 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / release

Dereference of a possibly null reference.

Check warning on line 406 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / release

Dereference of a possibly null reference.
if (!hasRecompile)
{
var names = string.Join(", ", unsnifffedParams.Select(p => p.Name));
Expand All @@ -367,11 +417,9 @@
}
}

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

// Rule 27: OPTIMIZE FOR UNKNOWN in statement text
if (!cfg.IsRuleDisabled(27) && !string.IsNullOrEmpty(stmt.StatementText) &&
Expand Down Expand Up @@ -1221,7 +1269,7 @@
// Rule 28: Row Count Spool — NOT IN with nullable column
// Pattern: Row Count Spool with high rewinds, child scan has IS NULL predicate,
// and statement text contains NOT IN
if (!cfg.IsRuleDisabled(28) && node.PhysicalOp.Contains("Row Count Spool"))

Check warning on line 1272 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test

Dereference of a possibly null reference.

Check warning on line 1272 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / build-and-test

Dereference of a possibly null reference.

Check warning on line 1272 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / release

Dereference of a possibly null reference.

Check warning on line 1272 in src/PlanViewer.Core/Services/PlanAnalyzer.cs

View workflow job for this annotation

GitHub Actions / release

Dereference of a possibly null reference.
{
var rewinds = node.HasActualStats ? (double)node.ActualRewinds : node.EstimateRewinds;
if (rewinds > 10000 && HasNotInPattern(node, stmt))
Expand Down Expand Up @@ -1445,41 +1493,6 @@
return Regex.IsMatch(side, @"\[[^\]@]+\]\.\[");
}

/// <summary>
/// Detects CTEs that are referenced more than once in the statement text.
/// Each reference re-executes the CTE since SQL Server does not materialize them.
/// </summary>
private static void DetectMultiReferenceCte(PlanStatement stmt)
{
var text = stmt.StatementText;
var cteMatches = CteDefinitionRegex.Matches(text);
if (cteMatches.Count == 0)
return;

foreach (Match match in cteMatches)
{
var cteName = match.Groups[1].Value;
if (string.IsNullOrEmpty(cteName))
continue;

// Count references as FROM/JOIN targets after the CTE definition
var refPattern = new Regex(
$@"\b(FROM|JOIN)\s+{Regex.Escape(cteName)}\b",
RegexOptions.IgnoreCase);
var refCount = refPattern.Matches(text).Count;

if (refCount > 1)
{
stmt.PlanWarnings.Add(new PlanWarning
{
WarningType = "CTE Multiple References",
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.",
Severity = PlanWarningSeverity.Warning
});
}
}
}

/// <summary>
/// Verifies the OR expansion chain walking up from a Concatenation node:
/// Nested Loops → Merge Interval → TopN Sort → [Compute Scalar] → Concatenation
Expand Down
Loading
Loading