Add the missing CHANGELOG entry for #1667 - #1672
Merged
Merged
Conversation
The merged fix had no [Unreleased] entry. Records the behaviour change as well as the fix: that one row's sql_variant base type goes from binary(8) to nvarchar(20), so anyone querying the view directly and converting to varbinary needs to adjust -- though the old value was garbled and therefore unusable, which is the bug being fixed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-and-view-smoke # Conflicts: # CHANGELOG.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1667, which merged without an
[Unreleased]entry.Records the behaviour change alongside the fix: for that one pivoted row the
sql_variant's base type goes frombinary(8)tonvarchar(20), so a caller doingCONVERT(varbinary(8), metric_value)needs adjusting. In practice the old value was garbled and therefore unusable — which is the bug — but it is a consumer-visible contract change and shouldn't ship silently.The other follow-up I proposed is deliberately NOT here
My review of #1667 suggested adding a smoke
SELECTperreport.*view to CI, on the grounds that.github/sql/ci_validate_installation.sqlonly checksOBJECT_ID(...) IS NULLand never executes the views. I'm dropping that, for two reasons.It would not have worked. I tested it against SQL Server 2022 with a minimal reproduction of the #1635 bug class — a
bitcolumn compared toN'OFF'inside a view:SELECT TOP (0)on an empty tableSELECT TOP (1)on an empty tableSELECT TOP (1)with one row presentCI validates a fresh install, so every collector table is empty and the error never fires. Catching this class needs representative seeded rows across all 43 views, not a smoke select. My review claim that "a single
SELECT TOP (1)per reporting view would close both" was wrong.And it protects deprecated code. These views live in
install/, which serves the retired Full Dashboard — no longer built into release artifacts, bug-fix-only. Building new CI infrastructure for it is the wrong investment, and the cheap version doesn't even work.The gap is real and is now described accurately on #1660, rather than half-closed with something that would have given false confidence.
🤖 Generated with Claude Code