parser: promote inline bare column-level KEY to PRIMARY#94
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
=======================================
Coverage 87.00% 87.00%
=======================================
Files 30 30
Lines 3271 3271
=======================================
Hits 2846 2846
Misses 261 261
Partials 164 164 ☔ 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 updates the desired-SQL parser to match MySQL’s behavior for inline column-level bare KEY by promoting it to a real PRIMARY KEY (including NOT NULL enforcement), preventing silent loss of user intent during parse → plan/apply round-trips.
Changes:
- Treat
sqlparser.ColKey(bare inlineKEY) the same asColKeyPrimaryinapplyInlineColumnKey, including NOT NULL forcing on the PK column. - Expand/clarify parser doc comments to describe the KeyOpt enum mapping and rationale.
- Add unit + end-to-end apply fixtures to pin the behavior and prevent regressions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
parser/parser.go |
Promotes bare column-level KEY to PRIMARY KEY in the parser, matching MySQL behavior and enforcing NOT NULL. |
parser/parser_test.go |
Adds unit tests validating PK/NOT NULL promotion, DEFAULT preservation, and precedence behavior. |
testdata/apply/inline_column_bare_key.yml |
Adds an integration fixture ensuring end-to-end round-trip emits the promoted PRIMARY KEY and stays drift-free. |
AGENTS.md |
Updates scope documentation (but currently contains an inconsistency with actual parser behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MySQL accepts `col INT KEY` as a synonym for `col INT PRIMARY KEY`: the column gets a real PRIMARY KEY index, NOT NULL is forced on the column, and a second column carrying bare `KEY` errors at apply time with `Multiple primary key defined`. Probed: CREATE TABLE t (id INT KEY); -- SHOW CREATE TABLE t: -- CREATE TABLE `t` ( -- `id` int NOT NULL, -- PRIMARY KEY (`id`) -- ) myschema's parser previously fell through on `sqlparser.ColKey` (the vitess enum value for bare KEY), silently dropping the user's PK declaration: the model had no PRIMARY constraint, no PRIMARY index, no NOT NULL on the column. The emitted CREATE TABLE matched the broken model — apply produced a PK-less table, and the round-trip looked clean because catalog and desired both agreed on "no PK." The user's intent disappeared without a single warning. Fix: add `sqlparser.ColKey` to the `ColKeyPrimary` case in `applyInlineColumnKey` so both forms share the PK promotion path. The existing skip-if-PRIMARY-already-set guard handles the "two columns each with KEY" / "KEY then PRIMARY KEY" precedence the same way it does for the `ColKeyPrimary × ColKeyPrimary` case (first column wins; MySQL still rejects at apply time). Also rewrite the comments on `applyInlineColumnKey` and the matching call site in `parseCreateTable`. The original comments claimed `ColKey, ColKeySpatialKey, and ColKeyFulltextKey aren't valid MySQL column-level attributes`, which was wrong on two counts: - `ColKey` is valid (PK synonym, this PR). - `ColKeySpatialKey` / `ColKeyFulltextKey` are unreachable from any valid SQL input — vitess's own grammar rejects the column-level shapes (`col GEOMETRY SPATIAL KEY`, `col TEXT FULLTEXT KEY`) with a syntax error before `applyInlineColumnKey` is reached. The enum values exist as defensive carry-over; we leave the arms unhandled rather than add errors for an unreachable path (per CLAUDE.md "Don't add validation for scenarios that can't happen"). Tests: - `TestParseInlineColumnBareKeyIsPrimaryKey` (parser): three sub-tests pin (a) basic promotion to PRIMARY constraint / index / NOT NULL, (b) coexistence with DEFAULT, (c) precedence with a later ColKeyPrimary on a different column. - `testdata/apply/inline_column_bare_key.yml`: end-to-end round-trip — desired uses `id INT KEY`, applied DDL emits the promoted PRIMARY KEY clause, and `verify_no_drift: true` confirms re-plan after apply is empty. AGENTS.md "In scope (v1)" now spells out that the bare `KEY` form is honoured as a PRIMARY KEY synonym. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c79696c to
1ca2483
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 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.
Summary
MySQL accepts
col INT KEYas a synonym forcol INT PRIMARY KEY(column gets a real PRIMARY KEY, NOT NULL is forced, two columns each carrying bareKEYerrors withMultiple primary key definedat apply). Probed:```
CREATE TABLE t (id INT KEY);
-- SHOW CREATE TABLE t:
-- CREATE TABLE `t` (
-- `id` int NOT NULL,
-- PRIMARY KEY (`id`)
-- )
```
myschema's parser previously fell through on `sqlparser.ColKey` (vitess's enum for bare column-level
KEY), silently dropping the user's PK declaration. The emitted DDL had no PRIMARY KEY and no NOT NULL on the column, the round-trip looked clean (catalog and desired both agreed "no PK"), and the user's intent disappeared without a warning.Discovered while auditing "places where myschema accepts SQL that MySQL rejects" (the inverse audit motivated by PR #92).
Fix
KEYis valid (this PR fixes it), and `ColKeySpatialKey` / `ColKeyFulltextKey` are unreachable from any valid SQL because vitess's own grammar rejects the column-level shapes.Tests
Test plan
🤖 Generated with Claude Code