catalog: normalise empty-string DEFAULT for fixed-width BINARY#34
Merged
Conversation
Closes the BINARY follow-up filed in PR #33's TODO.md entry. A column declared with `BINARY(N) NOT NULL DEFAULT ''` apply-looped the same way string-shaped types did before PR #33: every post-apply plan re-emitted `MODIFY COLUMN … DEFAULT ''` because the parser side stored the quoted empty literal while the catalog returned MySQL's sentinel for that case. The actual sentinel — verified by probing `information_schema.COLUMNS.COLUMN_DEFAULT` on both 8.0 and 9.4 — is the literal two-character string `0x`, independent of N. (TODO.md's prior guess of zero-padded `0x00000000…` for non-zero N was wrong; MySQL collapses every fixed-width BINARY empty default to the same `0x` regardless of width.) Recognise that sentinel in `normalizeColumnDefault` for `binary(...)`-prefixed types and rewrite it to `''` so the round-trip closes. `columnTypeAllowsEmptyStringDefault` keeps excluding BINARY: that function only enumerates types that take the bare-empty-string path, and BINARY now goes through the new `0x` path instead. Tests: - `TestColumnDefaultBinaryEmptyStringNormalisation` replaces the old `…IsHex` regression that pinned the limitation. Exercises BINARY(1), BINARY(4), BINARY(16) to lock in that the sentinel is N-independent. - `testdata/apply/empty_string_default_no_drift.yml` gains a BINARY(8) column so `verify_no_drift: true` proves the round-trip end-to-end. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #34 +/- ##
==========================================
+ Coverage 73.38% 73.40% +0.02%
==========================================
Files 27 27
Lines 2149 2151 +2
==========================================
+ Hits 1577 1579 +2
Misses 412 412
Partials 160 160 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR closes the remaining empty-string default drift for fixed-width BINARY(N) columns in the catalog layer, aligning catalog defaults with parser output so repeated plan/apply cycles stay stable. It extends the earlier empty-string default normalization work to the one remaining MySQL type family that still re-emitted MODIFY COLUMN after apply.
Changes:
- Update
normalizeColumnDefaultto rewrite theinformation_schema0xsentinel for emptyBINARY(N)defaults to''. - Replace the old catalog regression test with coverage for multiple
BINARYwidths and their normalized empty default. - Extend the YAML apply no-drift fixture to cover
BINARY(8)and remove the resolved follow-up item fromTODO.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
testdata/apply/empty_string_default_no_drift.yml |
Adds fixed-width BINARY(8) to the end-to-end no-drift regression fixture. |
catalog/tables.go |
Implements the catalog-side normalization of BINARY empty defaults from 0x to '' and updates related documentation comments. |
catalog/catalog_test.go |
Replaces the prior limitation test with assertions that multiple BINARY(N) sizes now normalize correctly. |
TODO.md |
Removes the now-resolved correctness bug entry for fixed-width BINARY DEFAULT '' drift. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
winebarrel
added a commit
that referenced
this pull request
May 3, 2026
case Previous wording said the empty-string default was the *only* type-aware path in `normalizeColumnDefault`. That was true at one point but PR #34 added a second type-aware branch: a fixed-width `BINARY(N)` column with `DEFAULT ''` surfaces from MySQL as the literal two-character string `"0x"` (independent of N), and the catalog rewrites that sentinel back to `''` when the type-name starts with `binary`. Restructure the bullet into a short three-case list (non-empty type-agnostic / empty-string type-aware / BINARY(N) "0x" type-aware) so the docs match the code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Closes the BINARY follow-up filed in PR #33's TODO.md entry.
A column declared with
BINARY(N) NOT NULL DEFAULT ''apply-looped the same way string-shaped types did before PR #33: every post-apply plan re-emittedMODIFY COLUMN … DEFAULT ''because the parser side stored the quoted empty literal while the catalog returned MySQL's sentinel for that case.Fix
The actual sentinel — verified by probing
information_schema.COLUMNS.COLUMN_DEFAULTon both 8.0 and 9.4 — is the literal two-character string0x, independent of N. (TODO.md's prior guess of zero-padded0x00000000…for non-zero N was wrong; MySQL collapses every fixed-width BINARY empty default to the same0xregardless of width.)normalizeColumnDefaultnow recognises that sentinel forbinary(...)-prefixed types and rewrites it to''so the round-trip closes.columnTypeAllowsEmptyStringDefaultkeeps excluding BINARY: that function only enumerates types that take the bare-empty-string path, and BINARY now goes through the new0xpath instead.Tests
catalog/catalog_test.go::TestColumnDefaultBinaryEmptyStringNormalisationreplaces the old…IsHexregression that pinned the prior limitation. Exercises BINARY(1), BINARY(4), BINARY(16) so the N-independent sentinel is locked in.testdata/apply/empty_string_default_no_drift.ymlgains a BINARY(8) column soverify_no_drift: trueproves the round-trip end-to-end alongside the existing string-shaped types.Verified on mysql 8.0 + 9.4: full Go suite, YAML harness, scenario suite all green.
Test plan
make test(8.0)make test-mysql9(9.4)make test-scenario(8.0)make lint🤖 Generated with Claude Code