Skip to content

Repro scripts admit vector(n,float16) and declare each parameter once (#4613) - #4626

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/4613-repro-script-ps592
Sep 28, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/4613-repro-script-ps592

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Fixes #4613
Refs erikdarlingdata/PerformanceStudio#592

Why

A Query Store plan's generated repro script left out SQL Server 2025's half-precision vector(…,float16) parameters, because the data-type check the previous fix (#4567) installed didn't admit a two-part (n,name) suffix. Separately, a batch whose statements share a parameter — including auto-parameterized @0/@1 names — declared that parameter once per statement and failed with "The variable name '@id' has already been declared" or "Must declare the scalar variable @1".

What changes

Ports erikdarlingdata/PerformanceStudio#592 (merged 6e52102) into PerformanceMonitor.PlanAnalysis/ReproScriptBuilder.cs:

  • IsValidDataType: the suffix now accepts (n), (max), (p,s), or (n,name) — the last form is what admits vector(3,float16).
  • Parameters are grouped by name (case-insensitively) before declaring: each name is declared once. A name that different statements type differently is left out of the declaration list, with a warning naming it.
  • When the statements that share a parameter carry different compiled values, the script uses the first usable one and warns that the values disagreed.
  • The "no parameters" placeholder comment now distinguishes an unparameterized query from one whose parameters all failed to declare (/* No parameters declared: see the warnings above */).
  • IsSafeLiteral's numeric-literal check switched from \d to [0-9], matching PerformanceStudio, so a compiled value using another script's digits (which \d also matches but T-SQL doesn't read as numbers) still becomes a ? placeholder.

Hunk-by-hunk

  1. validParameters/grouping/conflictingNames/declarableGroups/safeParameters/differingValueNames block — ported verbatim (adapted variable names already matched).
  2. droppedCount now measured against validParameters, plus the two new warnings (conflicting types, differing values) — ported.
  3. The "no parameters" placeholder comment split into two cases — ported.
  4. IsValidParameterName's \A…\z anchors and doc comment — already in PM from Plan analysis: the repro script matches PerformanceStudio's (#4564) #4567; not reapplied (would be a no-op).
  5. IsValidDataType's new suffix grammar (adds the (n,name) alternative) — ported.
  6. IsSafeLiteral's three regexes switched to \A…\z — already in PM from Plan analysis: the repro script matches PerformanceStudio's (#4564) #4567. Only the \d → [0-9] change in the numeric branch was new; ported that one line.
  7. Test file (+227 lines) — ported as a new class (see below); PS's ParseScript call in each test isn't present in PM (no T-SQL parser dependency in this project), so it's omitted; every Assert.Contains/DoesNotContain line is kept.

Everything else in PS's diff was already on PM's side from #4567 (the \A/\z anchor switch, CommentSafe, no USE line on control characters, the leading-digit parameter name pattern).

Consumers

Lite (Lite/Analysis) and the Darling Viewer both build against the shared ReproScriptBuilder and needed no changes; their builds are 0 errors / 0 warnings with this change.

Test plan

New class Darling/Darling.Tests/PlanSync4613ReproScriptTests.cs ports PerformanceStudio's new tests (adapted: no ParseScript helper).

RED on origin/dev (4db597907, worktree at that commit): 8 of 28 tests in the new class failed, including the vector(3,float16) type-kept pin and the parameter-dedup pins. All 8 compiled fine on dev (no new members needed) — a genuine runtime RED, not a compile failure.

Mutation: reverted IsValidDataType to dev's pattern (no (n,name) suffix) against the otherwise-fixed code — vector(3,float16) and 3 other malformed-type pins went RED (4 of 28), confirming the pin is load-bearing. Reverted back; rebuilt green.

GREEN after the full port:

Darling.Tests  Total: 151, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0, Time: 4.346s

(classes run: PlanSync4613ReproScriptTests, PlanSync4564ReproScriptTests, ReproScriptBuilderHardeningTests, DocCommentHygieneTests, CommentFilterAdoptionTests)

Builds, 0 warnings / 0 errors: Darling.Tests, Lite.Tests, PerformanceMonitor.Darling.Viewer.

Lite.Tests can't run in-process on macOS (WindowsBase discovery); no Lite test touches ReproScriptBuilder directly, so none were run or skipped for this change.

CHANGELOG

SECTION: Fixed
ENTRY: - The repro script declares each parameter once ([#4626]) - When a batch's plan lists the same parameter, or an auto-parameterized @0/@1, for more than one statement, the repro script declared it once per statement and failed to run. Each name is now declared once; a name the statements give different types is left out with a warning, and a warning says when the statements' compiled values differ. Matches PerformanceStudio.
REF: [#4626]: #4626

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review September 28, 2026 19:06
@erikdarlingdata
erikdarlingdata merged commit e55416e into dev Sep 28, 2026
15 of 16 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/4613-repro-script-ps592 branch September 28, 2026 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant