parser: switch desired-side parse to ParseStrictDDL (close vitess partial-DDL silent swallow)#83
Merged
Merged
Conversation
Closes the High-severity TODO item from the PR #81 silent-drop audit. vitess's `Parser.Parse` calls `Parse2`, which has a partial-DDL fallback: when yacc fails but the tokenizer has a half-built AST, Parse2 logs `WARN ignoring error parsing DDL …`, sets `FullyParsed(false)` on the partial DDL, and returns **err=nil** with that partial AST. myschema's desired-side parse loop (parser.go) treated err=nil as success and handed the half-built AST to `parseCreateTable`. For inputs like `CREATE TABLE t (id INT STORAGE DISK, PRIMARY KEY (id));` (NDB-only `STORAGE DISK` that vitess doesn't grammar-parse) the partial AST was effectively empty, and `myschema plan` emitted `CREATE TABLE t ();`. apply against a multi-statement plan would half-apply: tables ahead of the bad one succeed, the bad one fails on MySQL syntax error, leaving the database in an inconsistent state. Silent data loss, not just feature drop. vitess provides `ParseStrictDDL` for exactly this case — same parse path but no partial-DDL fallback; yacc failure surfaces as the tokenizer's LastError. One-line swap in parser.go's main desired-side parse loop: - stmt, err := p.Parse(piece) + stmt, err := p.ParseStrictDDL(piece) Other call sites stay on `Parse`: - The `-- myschema:execute <check-sql>` validator parses a SELECT, not DDL — partial-DDL fallback never fires there. - partition.go and view.go parse SQL myschema *synthesises*; we control the input shape, no user-supplied syntax error can reach them. Test: TestParseSQL_PartialDDLSurfacesAsError pins the new behaviour against the `STORAGE DISK` repro from the audit. The previous-WARN noise is gone; the same input now returns a real Go error that includes vitess's syntax-error diagnostic. TODO.md: drop the High item now that the fix is in. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
TestParseSQL_PartialDDLAbortsBatch pins the realistic shape of the audit's silent-data-loss concern: a `good, bad, good` batch must abort the whole parse rather than returning a ParseResult with the two good tables and a half-built `bad`. Before the ParseStrictDDL switch, the loop would register `good_a`, swallow the syntax error in `bad`, register `good_c`, and `apply` would half-apply the batch. Asserts: - err is non-nil - ParseResult is nil (no partial leak) - the wrapped error preserves vitess's `STORAGE` diagnostic so the user can find the offending statement Complements TestParseSQL_PartialDDLSurfacesAsError (single-stmt case). Both fail without the ParseStrictDDL fix. 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 #83 +/- ##
=======================================
Coverage 86.62% 86.62%
=======================================
Files 30 30
Lines 3268 3268
=======================================
Hits 2831 2831
Misses 269 269
Partials 168 168 ☔ 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 hardens desired-schema SQL parsing by switching the main parse loop to vitess’s ParseStrictDDL, preventing vitess’s partial-DDL fallback from silently returning an incomplete AST (and thereby avoiding incorrect DDL like CREATE TABLE t (); being emitted/applied).
Changes:
- Use
Parser.ParseStrictDDLinParseSQL’s statement loop to surface partial-DDL conditions as errors. - Add regression tests covering both single-statement and multi-statement “partial DDL” repro cases to ensure parsing fails fast and returns no partial
ParseResult. - Remove the corresponding “High” TODO entry now that the issue is addressed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| TODO.md | Removes the “High” TODO item describing vitess partial-DDL swallow now that it’s fixed. |
| parser/parser.go | Switches desired-side parsing from Parse to ParseStrictDDL with rationale in comments. |
| parser/parser_test.go | Adds tests to ensure partial-DDL becomes a hard error and aborts multi-statement batches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Closes the High-severity TODO item from the PR #81 silent-drop audit. vitess's
Parser.ParsecallsParse2, which has a partial-DDL fallback: when yacc fails but the tokenizer has a half-built AST, Parse2 logsWARN ignoring error parsing DDL …, setsFullyParsed(false)on the partial DDL, and returns err=nil with that partial AST.myschema's desired-side parse loop (
parser/parser.go) treatederr=nilas success and handed the half-built AST toparseCreateTable. For inputs likeCREATE TABLE t (id INT STORAGE DISK, PRIMARY KEY (id));(NDB-onlySTORAGE DISKthat vitess doesn't grammar-parse) the partial AST was effectively empty, andmyschema planemittedCREATE TABLE t ();. apply against a multi-statement plan would half-apply: tables ahead of the bad one succeed, the bad one fails on a MySQL syntax error, leaving the database in an inconsistent state. Silent data loss, not just feature drop.vitess provides
ParseStrictDDLfor exactly this case — same parse path but no partial-DDL fallback; yacc failure surfaces as the tokenizer'sLastError. One-line swap in the main desired-side parse loop:Other call sites stay on
Parse:-- myschema:execute <check-sql>validator parses a SELECT, not DDL — partial-DDL fallback never fires there.partition.goandview.goparse SQL myschema synthesises; we control the input shape, so no user-supplied syntax error can reach them.TODO.md: drop the High item now that the fix is in.Test plan
TestParseSQL_PartialDDLSurfacesAsError— single-statement repro:CREATE TABLE bad (id INT NOT NULL STORAGE DISK, …);must error and the wrapped error preserves vitess'sSTORAGEdiagnostic.TestParseSQL_PartialDDLAbortsBatch— multi-statement repro:good, bad, goodbatch aborts the whole parse (nil ParseResult, no partial leak). This is the realistic shape of the half-apply risk.ParseStrictDDLswap — verified by running them against the previousParseshape during development.make lintclean,make testall six packages green (no existing fixture relied on the partial-DDL tolerance).WARN ignoring error parsing DDLslog noise is gone; the same input now exits non-zero with a usable error message at plan time.🤖 Generated with Claude Code