Repository navigation
fix(ddl): carry a parenthesised literal DEFAULT, and cover every ALTER clause - #1577
Open
minguyen9988 wants to merge 7 commits into
Open
minguyen9988 wants to merge 7 commits into
minguyen9988 wants to merge 7 commits into
Conversation
MySQL parses a DEFAULT supplied in parentheses through the '(' expression ')' rule of its grammar, so a plain constant arrives wrapped: ALTER TABLE t ADD COLUMN c TINYINT(1) NOT NULL DEFAULT (0).
The translator treated any default without a constant child as a computed expression and refused the statement, stopping replication on a value the replica can reproduce exactly. MySQL back-filled every pre-existing row with that literal, and a ClickHouse DEFAULT of the same literal re-evaluates to the same value for every part that lacks the column.
Unwrap enclosing parentheses when they enclose one literal, with any number of nested parentheses and at most one unary sign. A parenthesised expression that is not a single literal -- DEFAULT (UUID()), DEFAULT (1 + 2), DEFAULT (CURRENT_DATE) -- keeps the existing loud refusal: those are values MySQL computed that the replica cannot reproduce for rows that already exist.
- DefaultLiteralTranslator: parenthesizedLiteral and soleConstant walk the expression tree; constantLiteral holds the previous literal logic.
- Sign refuses a second unary operator, so '- -1' is refused rather than emitted as '--1', which starts a comment in ClickHouse.
- Spec 06.04 section 3.2.1 states the rule; 3.2.2 rule 3 points at it.
- Tests cover single, nested, signed, string, NULL, double-quoted and backticked-with-position forms, plus the nested-expression refusal.
Co-Authored-By: Claude Code <noreply@anthropic.com>
A parenthesised literal default reached production untranslated and stopped replication. The unit test named that one shape and no other, so the gap was not the missing case -- it was that coverage was chosen by hand and nothing noticed what it left out. Cover the clause space by construction instead. The Debezium ANTLR grammar defines 50 AlterBy* alternatives of alterSpecification. AlterTableDdlVariationMatrixTest reflects the grammar at run time, holds one row per alternative carrying a clause that reaches it, and fails when an alternative has no row. A grammar bump therefore fails the build until the new clause is classified instead of silently shipping untranslated. Every row also asserts the alternative the parser actually enters, whether translation emits or deliberately skips, and that no clause throws. Two alternatives are not reachable at all. tableOption is alternative 1 of alterSpecification and its charset production subsumes every form alterByDefaultCharset accepts, so the latter is shadowed and unreachable from any input. It is declared as such, and a second test fails if a row claims it, so a grammar reorder that makes it reachable cannot leave it untested. A parenthesised literal is transparent, pinned over 17 literal forms across INT, TINYINT(1), BIGINT, DECIMAL, DOUBLE, VARCHAR, CHAR and DATE, with and without parentheses. tests/alter_ddl_matrix.py applies the same clause set to a live MySQL and requires a row written after each DDL to arrive in ClickHouse, so a clause that stalls the stream or emits a statement ClickHouse rejects fails the run. Registered in both regression_auto.py and regression_auto_replicated.py, and the raw-clause steps in steps/alter.py give it its verbs. Spec 06.04 section 5 names all of it. Destructive-Op-Check: sites=4; result=pass Co-Authored-By: Claude Code <noreply@anthropic.com>
Guard requires a DESTRUCTIVE comment at every fixture quoting DDL that drops or deletes. The fixtures are strings parsed only; nothing runs. Destructive-Op-Check: sites=18; result=pass
Two tests that read the coverage set from the parser the connector links against instead of a hand-written list, so a statement family or grammar rule nobody classified fails the build. StatementSurfaceCoverageTest: every ddlStatement alternative the shipped MySqlParser declares must be either handled by MySqlDDLParserListenerImpl (proved by an enter* method on that class) or named in INTENTIONALLY_UNHANDLED; the two sets must be disjoint; every unhandled family must parse without throwing; and the other sqlStatement groups (transaction, replication, prepared, administration, utility) must parse silently too. A parser upgrade that adds a DDL form fails here until someone decides which bucket it belongs in. GrammarRuleCoverageTest: walks a corpus of more than 300 valid MySQL statements through the production parse path (MySqlDdlNormalizer, MySqlLexer with CaseChangingCharStream, then root()) and records every parser rule the walk visits. Every rule in MySqlParser.ruleNames must be visited or named in NOT_EXERCISED with the alternative that shadows it. An ATN reachability walk reports zero structurally unreachable rules, so 'unreachable' alone is never an accepted reason; the four entries are grammar alternatives ANTLR can never select (setAutocommitStatement, multipleUpdateStatement, partitionDefinerVector, unionParenthesis), each with the shipped form that wins instead. A rule that becomes reachable fails the stale check. Both classes were run green in the sandbox against the full parser suite before this commit. Destructive-Op-Check: sites=21; result=pass Co-Authored-By: Claude Code <noreply@anthropic.com>
Adds matrix, grammar, real-world and surface corpora. Drops the invalid RENAME TABLES head; fixes AUTOEXTEND_SIZE with quoted sizes. Destructive-Op-Check: sites=37; result=pass
No functional change. The brace and first statement shared a line; the partition-key javadoc sat above the wrong method. Destructive-Op-Check: sites=0; result=pass
Resolve the expectations in MySqlDDLParserListenerImplTest against the merged parser. The two sides changed disjoint behaviour, so each conflicted expectation takes both: the base branch's UTC-qualified DateTime64 rendering and this branch's DEFAULT emission. PARTITION BY keeps the single space this branch emits, matching Constants.PARTITION_BY. Verified on the merged tree: 167 tests in MySqlDDLParserListenerImplTest, 0 failures, 0 errors. Co-Authored-By: Claude Code <noreply@anthropic.com> Jinja-Render-Check: rendered=3; formats=yaml,xml,systemd; result=pass Destructive-Op-Check: sites=29; result=pass
This branch has not been deployed
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.
Alrea
62B0
dy 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.
Problem
A MySQL source DDL of the form
is rejected by the DDL translator with:
DDLReplicationException is terminal, so the connector stops applying DDL and every later statement in the stream queues behind it. One DEFAULT (0) is enough to halt replication for a table.
MySQL parses a parenthesised default through its bracketed expression production, so DEFAULT (0) never reaches the literal branch: DefaultValueContext.constant() is null, literalDefault() returns null, and resolveDefault() treats null as a value computed on the source. But (0) is not computed. It is the literal 0 carrying a redundant pair of brackets. MySQL back-filled the pre-existing rows with that literal at ALTER time, and DEFAULT 0 in ClickHouse reproduces it exactly. Refusing it stops a stream over a value the replica can reproduce perfectly.
The deeper problem is that the unit test named that one shape and no other. Coverage was chosen by hand, and nothing noticed what it left out, so a clause as ordinary as DEFAULT (0) shipped untranslated. Chasing that to its end turned up a second problem underneath: the MySQL grammar the connector links against is older than the MySQL surface it is fed, and several accepted constructs were unreachable through it at all, aborting the whole statement.
Change
1. Carry a parenthesised literal default
DefaultLiteralTranslator now unwraps parentheses that enclose nothing but a single literal, and reuses the existing literal translation.
The new soleConstant overloads descend only through the labelled alternatives that can hold a literal (PredicateExpressionContext, ExpressionAtomPredicateContext, ConstantExpressionAtomContext), plus UnaryExpressionAtomContext for a single sign and NestedExpressionAtomContext for one more level of parentheses. Anything else returns null and keeps the existing refusal.
2. Translate every default literal shape MySQL actually stores
Measured on 8.0.41, then encoded in the translator so the replica holds the value the source holds rather than a plausible approximation:
3. Bridge the grammar gaps instead of aborting on them
MySqlDdlNormalizer (new) rewrites a few MySQL spellings the bundled grammar cannot parse into an equivalent spelling it can, before the parser sees the text. Text inside a string literal, a quoted identifier or a comment is never rewritten, and a statement that needs no rewrite is returned by identity. The shipped parser is a fixed compiled artifact, so there is no grammar file on the runtime path to extend.
MySQLDDLParserService routes every statement through the normalizer and recovers a statement kind it does not translate into a logged, non-fatal drop instead of an exception out of the parser.
4. Two type spellings that could not be replicated at all
5. Partition key emission
6. Coverage derived from the grammar, not chosen by hand
This is the part that answers the class of defect rather than the instance.
7. End-to-end matrices against a live pair of servers
All three are registered in both regression_auto.py and regression_auto_replicated.py, so the existing CI filters pick them up in the plain and the pooled job. steps/alter.py gains the raw-clause verbs they need.
8. Specification
Spec 06.04 gains the parenthesised-literal rule, a reworded refusal rule for expressions, and names every new test. The two shipped DDL compatibility tables are corrected for the partition key forms.
Commits
Read the diff, not only the log: the third commit message describes the smallest thing in it. That commit carries the normalizer, the parser service routing, the listener partition-key work, the type converter entries and most of the new unit classes.
Verification run on this head
The whole DDL parser test package, run offline with JDK 21 and Maven 3.9.11 in an isolated sandbox:
25 test classes executed, including every class this change adds or touches.
Per class, for the classes added or enlarged here:
The seven skips are pre-existing disabled tests, each naming a known defect rather than hiding one: a data-changing partition operation that is skipped as if loss-free, a failed sorting-key lookup treated as an unknown key, a failed nullability lookup that widens to Nullable, a generation expression that loses its token boundaries, missing ON CLUSTER on table-level statements in replicated mode, a TINYINT(1) mapping that collapses values, and one long-disabled partitioning test. They are not made to pass by this change and they are listed here so the count is not mistaken for a green board.
Mutation check: reverting the new default branch to return null makes testParenthesizedLiteralDefaultIsTranslated fail with exactly the production error text quoted above, and dropping a matrix row makes everyGrammarAlternativeHasAMatrixRow fail naming the alternative.
Not run here
The end-to-end matrices need a live MySQL and ClickHouse pair and were not executed in this environment; only their static checks and their registration were verified. The unit and matrix classes above are the executed evidence.
Notes
Generated with Claude Code (https://claude.com/claude-code)