model: full-attribute ordering pins for emitters#88
Merged
Conversation
Third item from the PR #84 postmortem audit. PR #84 pinned only INVISIBLE's slot (`AUTO_INCREMENT < INVISIBLE < COMMENT`) by index comparison; the rest of the emitter ordering still relied on Contains and was order-blind. Extend the index-comparison pins so a reorder of any emitter slot fails a specific edge in CI instead of silently shipping wrong-order DDL. - TestColumnDefSQLFullOrdering pins the full chain (CHARACTER SET → COLLATE → GENERATED → NOT NULL → DEFAULT → ON UPDATE → AUTO_INCREMENT → INVISIBLE → COMMENT). Sets every attribute on one Column even though some combinations (AUTO_INCREMENT + GENERATED) are semantically illegal — columnDefSQL is a pure formatter and treats fields independently, so the pin covers their slots regardless. - TestConstraintInlineSQLCheckEnforcedOrdering pins that the CHECK keyword body precedes the trailing NOT ENFORCED suffix. Single-suffix-pair test, runs through Table.SQL since constraintInlineSQL isn't exported. - TestIndexSQLAttributeOrdering pins Index.SQL's trailing attribute order (USING → INVISIBLE → COMMENT). KEY_BLOCK_SIZE isn't currently emitted (separate TODO entry); when wired in it'll need a slot here too. 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 #88 +/- ##
=======================================
Coverage 86.90% 86.90%
=======================================
Files 30 30
Lines 3277 3277
=======================================
Hits 2848 2848
Misses 263 263
Partials 166 166 ☔ 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 strengthens regression coverage around SQL emitter attribute ordering so that any future reordering of emitted tokens fails CI with a targeted, order-specific assertion (rather than silently producing wrong-order DDL).
Changes:
- Add a comprehensive ordering “pin” test for
ColumnDefSQLattribute emission order. - Add an ordering test ensuring
CHECK (...)appears before theNOT ENFORCEDsuffix in inline CHECK constraints viaTable.SQL(). - Add an ordering “pin” test for trailing
Index.SQL()attributes (USING → INVISIBLE → COMMENT).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| model/table_test.go | Adds ordering pin tests for column attribute emission and CHECK/NOT ENFORCED ordering in table SQL output. |
| model/index_test.go | Adds an ordering pin test for Index.SQL() trailing attribute emission order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The original doc claimed the test "mirrors what MySQL emits in SHOW CREATE TABLE", but the constructed column intentionally combines mutually-incompatible attributes (BIGINT + CHARACTER SET, AUTO_INCREMENT + GENERATED) so a single column exercises every formatter slot. Reword the header to make the slot-ordering intent explicit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR #88 review pointed out that TestColumnDefSQLFullOrdering pinned every other emitted attribute but not the generated-column storage modifier. A regression that moves STORED/VIRTUAL out of its post-`GENERATED ALWAYS AS (...)` slot — or flips STORED↔VIRTUAL for the wrong `Stored` value — would slip through. - Insert "STORED" between "GENERATED ALWAYS AS" and "NOT NULL" in the existing token list, and assert VIRTUAL is absent so a future emitter that prints both keywords doesn't pass. - Add TestColumnDefSQLFullOrderingVirtual as the Stored=false sibling that pins VIRTUAL's position. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
winebarrel
added a commit
that referenced
this pull request
May 6, 2026
Five of the six items from the PR #84 postmortem audit landed: - composed-state catalog tests → PR #86 - kitchen-sink round-trip → PR #87 - emitter ordering pins → PR #88 - diff cross-cutting → PR #89 - directive composition → PR #90 Drop the closed items. Keep `parseCreateTable` column-loop interactions under "Medium — test coverage gaps" — that one was scoped out of the kitchen-sink because the open shapes (inline `UNIQUE × DEFAULT`, inline `REFERENCES × NOT NULL × ON UPDATE`, inline `PK × column-level CHECK`) need direct parser unit tests, not a round-trip fixture. The kitchen-sink uses `email VARCHAR NOT NULL UNIQUE` (no DEFAULT), an explicit named CONSTRAINT FK (not inline REFERENCES), and a table-level CHECK (not the column-level shape that exercises the vitess promotion path), so the gap is real. The "Medium — silent diffs / fidelity gaps" section keeps the real open items, including the three new ones surfaced by the audit itself (REFERENCES auto-name, vitess-keyword identifier drift, CHECK referencing renamed column). 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
Third item from the PR #84 postmortem audit (TODO.md "Medium — test coverage audit", item 3). PR #84 pinned only INVISIBLE's slot (
AUTO_INCREMENT < INVISIBLE < COMMENT) by index comparison; the rest of the emitter ordering still relied onContainsand was order-blind. Extend the index-comparison pins so a reorder of any emitter slot fails a specific edge in CI instead of silently shipping wrong-order DDL.TestColumnDefSQLFullOrderingpins the fullcolumnDefSQLchain:CHARACTER SET → COLLATE → GENERATED → NOT NULL → DEFAULT → ON UPDATE → AUTO_INCREMENT → INVISIBLE → COMMENT. Sets every attribute on one Column even though some combinations (AUTO_INCREMENT + GENERATED) are semantically illegal —columnDefSQLis a pure formatter and treats fields independently, so the pin covers their slots regardless.TestConstraintInlineSQLCheckEnforcedOrderingpins that theCHECK (…)keyword body precedes the trailingNOT ENFORCEDsuffix. Single-suffix-pair test, runs throughTable.SQL()sinceconstraintInlineSQLisn't exported.TestIndexSQLAttributeOrderingpinsIndex.SQL()'s trailing attribute order:USING → INVISIBLE → COMMENT. (KEY_BLOCK_SIZEisn't currently emitted — see the separate TODO entry; when wired in it'll need a slot here too.)Test plan
make lintclean.make testall six packages green.🤖 Generated with Claude Code