fix(ddl): strip the auto-increment counter without mangling literals - #1300
Draft
aparajon wants to merge 2 commits into
Draft
fix(ddl): strip the auto-increment counter without mangling literals#1300aparajon wants to merge 2 commits into
aparajon wants to merge 2 commits into
Conversation
The schema-read paths stripped the AUTO_INCREMENT table counter with a regex substitution over the whole SHOW CREATE TABLE text, so any column default, column comment or identifier that happened to spell the keyword was rewritten too: DEFAULT 'AUTO_INCREMENT=123' became DEFAULT '', and a comment reading "reset AUTO_INCREMENT 1000 on rollover" lost its middle. A pull wrote the mangled DDL into the schema file, which then became the desired state and would generate an ALTER against a table nobody changed. StripTableAutoIncrement parses the statement with the real parser and only touches text when the AST actually carries a table-level counter, so a statement without one is returned byte for byte. When there is one, the counter's own span is removed and the result is parsed again and compared with the original in the parser's canonical form: anything else moving is an error, never a silent rewrite. The column-level AUTO_INCREMENT attribute is preserved — dropping it would produce a table whose ids no longer generate. The server's own formatting survives, so pulled and onboarded schema files keep their canonical SHOW CREATE layout. Extends RV-6 to the live-schema read path: DDL is now transformed through the dialect's parser rather than by matching its text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new DDL scanner treats any -- sequence as a line comment start, which can mis-handle valid SQL expressions and cause counter stripping to fail on otherwise valid CREATE TABLE statements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR replaces a regex-based AUTO_INCREMENT counter strip (which could mangle literals/identifiers) with a parser-guided, byte-precise removal of the table-level AUTO_INCREMENT=N option during live schema reads, keeping the server’s SHOW CREATE TABLE formatting intact.
Changes:
- Introduces
ddl.StripTableAutoIncrement, which locates and removes only the table-level counter and re-parses to verify nothing else changed. - Updates live-schema read paths (tern pull, Spirit engine fetch, LocalScale schema reads) to use the new stripping logic instead of Spirit’s stripped-auto-increment option.
- Adds focused unit + integration coverage to ensure literals/comments/identifiers containing
AUTO_INCREMENTare preserved while the counter is removed.
File summaries
| File | Description |
|---|---|
| pkg/ddl/auto_increment.go | Adds parser-verified, byte-precise stripping of the table-level AUTO_INCREMENT=N option. |
| pkg/ddl/auto_increment_test.go | Unit tests for counter stripping behavior and literal/comment preservation. |
| pkg/engine/spirit/spirit.go | Strips the table counter after loading live schema to avoid spurious diffs. |
| pkg/localscale/helpers.go | Switches LocalScale’s SHOW CREATE path to the new counter-strip function with error handling. |
| pkg/tern/local_client.go | Applies counter stripping during schemabot pull table DDL generation. |
| pkg/tern/local_client_integration_test.go | Integration test ensuring pull preserves literals/comments while dropping the table counter. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
MySQL opens a line comment on `--` only when a whitespace or control character follows the second dash, so `a--b` subtracts a negated value. Scanning every `--` as a comment skipped the rest of the line, and a generated column or default expression written that way hid the table options behind it: the counter parsed as a table option but could not be found in the statement text, failing the strip outright.
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.
The
AUTO_INCREMENTtable counter was stripped from live schema reads by a regex substitution over the wholeSHOW CREATE TABLEtext. The pattern's=is optional, so it matched the keyword anywhere it appeared — inside a column default, a column comment, or a quoted identifier.schemabot pullthen wrote the mangled DDL into the schema file. That file becomes the desired state, so the next plan proposes anALTERagainst a table nobody changed: setting a real default to'', or truncating a comment.What a pulled schema file looked like
Live table:
Before — two columns silently rewritten:
After — only the counter is gone:
ddl.StripTableAutoIncrementdecides with the parser and edits with byte precision:Two properties the shape is built around:
AUTO_INCREMENTattribute is preserved. Removing it would produce a table whose ids no longer generate..sqlfiles keep the canonicalSHOW CREATE TABLElayout. Restoring the modified AST instead would have reformatted every file belonging to a table that has a live counter, since the parser'sRestoreis not a pretty-printer.A statement whose counter cannot be located and removed cleanly is an error, not a passthrough — returning the input would write one instance's counter into a schema file.
Three call sites move off Spirit's
WithStrippedAutoIncrement: the pull path (pkg/tern), the differ's live-schema read (pkg/engine/spirit), and the shard-targeted read in LocalScale.RV-6 — this extends enforcement to the live-schema read path: DDL is now transformed through the dialect's parser rather than by matching its text. The entry's Enforced: line already names the Spirit
statementboundary, so the registry needs no change.Why not just take the upstream fix
block/spirit#1210 landed the same class of fix in
table.StripAutoIncrement, over the AST. It parses, deletes the option, and restores — which reformats the statement whenever a counter is present, because the parser'sRestoreis not a pretty-printer. On a realSHOW CREATE TABLEit returns:Literals survive, so the mangling is genuinely fixed. But every onboarded
.sqlfile for a table with a live counter would arrive on one line, uppercased, with_UTF8MB4'...'prefixes andDEFAULT CURRENT_TIMESTAMP()in place ofDEFAULT CURRENT_TIMESTAMP— while tables without a counter keep the server's layout, so a single onboarding would emit two different formats.That is a property of stripping after the read, and it cannot be avoided in
pkg/table:pkg/statementimportspkg/table, so the richer parse-and-splice used here cannot live there without inverting the dependency.This PR drops SchemaBot's use of
WithStrippedAutoIncrementaltogether, so the pull and onboard paths keep the server's formatting regardless of what the option does upstream.This PR was written by Claude Code (Opus 5).