parser/model/catalog/diff: round-trip SRID on spatial columns#100
Merged
Conversation
vitess parsed `cd.Type.Options.SRID` but `parseColumnDef` ignored it; the catalog never read `SRS_ID`. A desired schema with `SRID 4326` constants drifted forever — `dump → plan` was clean, but a hand-written desired had no path to reach the catalog's SRID. Wire SRID through every layer: - `model.Column.SRID *uint32` — *0 is a real declaration distinct from "unset" (catalog stores SRS_ID=0 for `SRID 0`, NULL for no clause). - `parser.parseColumnDef` reads `cd.Type.Options.SRID` (vitess *Literal whose Val is the integer text), parses to uint32. - `model.columnDefSQL` emits `SRID N` between NOT NULL and DEFAULT to match MySQL's SHOW CREATE TABLE column-attribute order. Plain `SRID N` (no `/*!80003 ... */` version-gated comment — myschema requires 8.0+ already). - `catalog.loadSpatialSRIDs` queries `information_schema.ST_GEOMETRY_COLUMNS`, populating Column.SRID for rows where SRS_ID is not NULL. - `diff.columnEqual` adds an SRID comparison so a desired-side SRID change fires a MODIFY COLUMN. Tests: - parser: TestParseColumnSRID (SRID 4326, SRID 0, unset). - model: TestColumnDefSQLSRID (nil → no emit, *0 → emit, slot pinning) plus TestColumnDefSQLFullOrdering extended with SRID between NOT NULL and DEFAULT. - catalog: TestSpatialSRIDRoundTrip (4326 / 0 / unset). - diff: TestColumnEqual sub-tests for value-differs, set-vs-unset, 0-vs-unset. - testdata/apply/spatial_srid_round_trip.yml: end-to-end fixture with all three states under verify_no_drift. Coverage on touched code: columnEqual / columnDefSQL 100%; loadSpatialSRIDs 82.4% (defensive error paths uncovered); parseColumnDef 94.9% (SRID branch fully covered). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR closes a schema round-trip fidelity gap for MySQL 8.0+ spatial columns by preserving and diffing SRID <n> attributes end-to-end (desired SQL ↔ model ↔ catalog ↔ dump/plan), so SRID declarations no longer silently disappear and drift forever.
Changes:
- Add
model.Column.SRID *uint32to representSRID N, preserving the important distinction between “unset” (nil) and “explicit SRID 0” (*uint32(0)). - Parse SRID from Vitess (
cd.Type.Options.SRID) into the model, emit it incolumnDefSQL, and include it indiff.columnEqualso SRID changes triggerMODIFY COLUMN. - Populate SRID from
information_schema.ST_GEOMETRY_COLUMNS.SRS_IDin the catalog, with unit/integration coverage plus an end-to-end apply fixture.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| TODO.md | Removes the resolved TODO item about SRID being dropped. |
| testdata/apply/spatial_srid_round_trip.yml | End-to-end apply fixture covering SRID=4326, SRID=0, and unset. |
| parser/parser.go | Parses cd.Type.Options.SRID into Column.SRID with uint32 validation. |
| parser/parser_test.go | Adds TestParseColumnSRID to pin parser behavior for SRID states. |
| model/column.go | Introduces Column.SRID *uint32 with semantics documented (nil vs explicit 0). |
| model/table.go | Emits SRID N in column definitions in MySQL-compatible attribute order. |
| model/table_test.go | Adds SRID emission tests and pins SRID’s ordering relative to other attributes. |
| diff/tables.go | Compares SRID as part of columnEqual so diffs surface SRID drift. |
| diff/tables_test.go | Adds SRID-focused ColumnEqual subtests (differs, set/unset, 0/unset). |
| catalog/tables.go | Loads SRID from information_schema.ST_GEOMETRY_COLUMNS into Column.SRID. |
| catalog/tables_test.go | Adds catalog round-trip test for SRID=4326, SRID=0, and unset. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
- Coverage 86.96% 86.86% -0.10%
==========================================
Files 30 30
Lines 3321 3357 +36
==========================================
+ Hits 2888 2916 +28
- Misses 266 270 +4
- Partials 167 171 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CI codecov/patch failed at 72.22% (target 75%). The biggest uncovered block in the patch was the SRID overflow error path in parseColumnDef — reachable from valid SQL (vitess accepts arbitrary integer text but Column.SRID is *uint32, so values > 4294967295 overflow at parse time). - TestParseColumnSRIDOverflow exercises the overflow path with SRID 4294967296 (uint32 max + 1). - Drop the redundant "column %s: " prefix from the error format. parseCreateTable already wraps with table+column context, so the message double-prefixed as "column g: column g: SRID..." before. Now the wrapped form reads "table app.t: column g: SRID..." cleanly. 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
TODO "Medium — silent diffs / fidelity gaps" item 2: `SRID ` on spatial columns was silently dropped. vitess parsed `cd.Type.Options.SRID` but `parseColumnDef` ignored it; the catalog never read `SRS_ID`. A hand-written desired schema with `SRID 4326` constants drifted forever — `dump → plan` happened to look clean only because dump round-trips through the same gap.
Changes
Wire SRID through every layer:
Tests
Coverage on touched code: `columnEqual` / `columnDefSQL` 100%; `loadSpatialSRIDs` 82.4% (defensive query/scan-error paths uncovered); `parseColumnDef` 94.9% (SRID branch fully covered).
Test plan
🤖 Generated with Claude Code