Skip to content

parser: promote inline bare column-level KEY to PRIMARY#94

Merged
winebarrel merged 1 commit into
mainfrom
parser-promote-bare-key-to-pk
May 6, 2026
Merged

parser: promote inline bare column-level KEY to PRIMARY#94
winebarrel merged 1 commit into
mainfrom
parser-promote-bare-key-to-pk

Conversation

@winebarrel

Copy link
Copy Markdown
Owner

Summary

MySQL accepts col INT KEY as a synonym for col INT PRIMARY KEY (column gets a real PRIMARY KEY, NOT NULL is forced, two columns each carrying bare KEY errors with Multiple primary key defined at 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

  • Add `sqlparser.ColKey` to the `ColKeyPrimary` case in `applyInlineColumnKey` so both shapes share the PK promotion path. The existing skip-if-PRIMARY-already-set guard handles the precedence cases (two columns each with KEY, KEY → PRIMARY KEY, etc.) the same way it does for the existing `ColKeyPrimary × ColKeyPrimary` case.
  • Rewrite the doc comments on `applyInlineColumnKey` and `parseCreateTable` to spell out the actual mapping. The originals claimed `ColKey, ColKeySpatialKey, ColKeyFulltextKey` were "not valid MySQL column-level attributes," which was wrong on two counts — bare KEY is 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

  • `TestParseInlineColumnBareKeyIsPrimaryKey` (parser): three sub-tests pin (a) basic PK + NotNull promotion, (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 (id)` clause, and `verify_no_drift: true` confirms re-plan after apply is empty.

Test plan

  • `make lint` clean.
  • `make test` all 6 packages green.
  • `make test-scenario` 5/5 passing.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 6, 2026 01:19
@codecov

codecov Bot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.00%. Comparing base (78eb2df) to head (1ca2483).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 inline KEY) the same as ColKeyPrimary in applyInlineColumnKey, 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.

Comment thread AGENTS.md Outdated
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>
@winebarrel winebarrel force-pushed the parser-promote-bare-key-to-pk branch from c79696c to 1ca2483 Compare May 6, 2026 01:26
@winebarrel winebarrel requested a review from Copilot May 6, 2026 01:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@winebarrel winebarrel merged commit fa1f2ba into main May 6, 2026
9 checks passed
@winebarrel winebarrel deleted the parser-promote-bare-key-to-pk branch May 6, 2026 01:32
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.

2 participants