statement/parser: minimal-paren canonicalization and binlog statement-class gaps

[morgo]

Minimal-paren canonicalization

Closes #1128

Ports pingcap/tidb@52b9a887b3b0: the opt-in format.RestoreSkipRedundantParentheses flag and the MySQL precedence table that decides whether a ParenthesesExpr may be dropped in its position. With the flag set, a + (b * c) restores as a + b * c, while (a + b) * c and a - (b - c) keep their parentheses — a same-precedence right child only drops them for operators that regroup safely. Default restore behaviour is unchanged, since the flag is opt-in.

Five operators need a rule of their own. BETWEEN, IN, LIKE, REGEXP and MEMBER OF look infix, but MySQL gives their operands fixed productions rather than expressions at their own precedence level, so they do not nest the way the table implies: dropping the parentheses either rebinds the expression, since the server reads a = b BETWEEN 1 AND 10 as a = (b BETWEEN 1 AND 10), or emits text it rejects outright, as with a IN (1,2) IN (3,4). Each operand position is held to its own production instead of the operator's precedence: subjects, BETWEEN's bounds and REGEXP's pattern are bit_exprs, so (a + b) IN (1,2) and a BETWEEN (b + 1) AND (c * 2) still minimize, while LIKE's pattern and MEMBER OF's document are simple_exprs, a level only COLLATE reaches. 26 shapes are pinned across the restore and canonicalizer tests, plus 3 pairs that must stay textually distinct.

pkg/statement's expression normalizer uses it, so a CHECK constraint now canonicalizes as CHECK (`a`=1 OR `b`=2 AND `c`=3) rather than the fully-parenthesized CHECK ((`a`=1) OR ((`b`=2) AND (`c`=3))). The existing strip-and-wrap pass stays: it erases the input's parenthesization so the text is a function of the parse tree alone, which is what makes the form canonical. Two shapes still can't converge textually, because MySQL rewrites the expression when storing it — NOT (a > 0 AND b > 0) comes back De Morgan'd, and s REGEXP '^y' as regexp_like(...).

Fix found along the way: the strip-and-wrap pass didn't re-wrap MEMBER OF, quantified comparisons, or COLLATE, so CHECK (a = (1 MEMBER OF (j))) was rendered as a = 1 MEMBER OF (j) — which reads left to right as the different (a = 1) MEMBER OF (j). Pre-existing, but it would have become a collision between the two forms under minimal parens.

Parser MySQL compatibility

pkg/change currently ignores binlog Query events it can't parse. Making that strict needs a parser that recognizes everything MySQL writes to the binary log, so the rest of the commits close the gap against the MySQL 9.6 grammar, measured on the mysql-test corpus (196,996 statements):

Pass rate 90.55% → 99.74%, panics 16 → 0. A 457-probe set of binlog-relevant clause forms went from 119 failing to 0.

Areas closed, roughly in the order the corpus surfaced them:

• A P0 panic: decimal literals wider than 81 digits crashed MyDecimal.FromString. A strict pkg/change must never panic on input the server accepted.
• DDL and the type system: INVISIBLE columns and ENGINE_ATTRIBUTE, VECTOR, DEFAULT (expr) as a full expression, all 41 charsets and their collations, tablespaces, logfile groups, spatial reference systems, ALTER VIEW, ALTER INSTANCE, and the remaining partition and LOAD DATA clauses.
• Access control: the multi-factor, random-password and RETAIN CURRENT PASSWORD surface of CREATE/ALTER USER, plus GRANT ... AS and REVOKE IF EXISTS.
• Administration: XA, HANDLER, CHECK/CHECKSUM/REPAIR TABLE, plugins and components, resource groups, CLONE, replication administration (CHANGE REPLICATION SOURCE, START/STOP REPLICA, SET PERSIST), and the EXPLAIN/SHOW variants.
• Expressions: JSON_TABLE and JSON_VALUE, WEIGHT_STRING, ODBC escapes, MATCH ... AGAINST, SELECT ... INTO, INTERVAL() arity, MEMBER with the OF omitted, CAST(x AS DOUBLE PRECISION), and REGEXP's pattern as a full bit_expr rather than a simple_expr — 'a' REGEXP 'a' + 'b' parses, 'a' LIKE 'a' + 'b' is a syntax error, on the fork and on the server alike.
• Stored programs, the largest single piece: procedures, functions, triggers and events with their full compound-statement bodies — DECLARE, handlers, cursors, IF/CASE/loops, SIGNAL, labels. pkg/change needs this because a routine body arrives in the binary log as one Query event. Worth 95.62% → 99.34% on its own.
• 9.x features: LIBRARY DDL, dollar-quoted strings, FLUSH option lists.
• Keywords follow live MySQL: OPTIMIZER_COSTS, SYSTEM, UNDO and JSON_TABLE become reserved, ~35 unreserved keywords are added, and unreserved keywords are split into the label/non-label sets MySQL uses.

The remaining 506

Not a backlog. Each one was replayed byte-exact against a live MySQL 9.7 through COM_STMT_PREPARE, which parses a statement without executing it: 504 come back ERROR 1064, so MySQL's own grammar rejects them too. The two that returned 1193 short-circuit on an unknown system variable before the server reaches the rest of the text — substituting a variable it does know gives 1064 as well.

They are mysqltest directives (246), literals made of raw sjis/big5/ucs2 test bytes (164), extraction artifacts such as multi-statement records and statements cut mid-literal (59), ANSI_QUOTES DDL (25), and SQL the test files run deliberately to check an error path (12).

So on the criterion that matters for a strict pkg/change — does the fork parse everything the server parses — this is 100%.

Validation

• Grammar shapes were validated against live MySQL 8.0.46 and 9.7.0 — clause order, rejection cases, and SHOW CREATE output shapes — rather than inferred from sql_yacc.yy alone. Where the two disagreed I followed the server: JSON_TABLE really is reserved, empty WHILE/REPEAT/LOOP bodies really are rejected, and x MEMBER (doc) really does read as MEMBER OF. Each class has round-trip restore tests in binlog_stmt_gaps_test.go.
• TestRoundTrip_ExpressionParenShapes runs 28 expression shapes both directions against MySQL 8.0.45: the shape as a person writes it must converge with MySQL's stored form, and the canonical text must apply as DDL and still converge after MySQL stores it. That second direction is what checks the ported precedence table against MySQL's own parser. TestCanonicalExprParensIsFixedPoint and TestCanonicalExprParensKeepsDistinctExpressionsDistinct cover it offline.
• The grammar generates with zero reported conflicts and the parser-regen CI gate (byte-identical regeneration from parser.y) passes; golangci-lint run reports 0 issues.

🤖 Generated with Claude Code

[Kiran01bm]

🤖 Review findings - created by Kiran's code review agent - for pull/1134, 7ccffd3.

Verdict: 3 findings — 1 blocking (paren canonicalization unsound for BETWEEN/IN/LIKE/MEMBER OF), 2 non-blocking (decimal-warning digit corruption, stale README table). CI: 13/13 checks pass at head 7ccffd3 (DCO Check, build x7 matrix jobs, build(gtid), build(nogtid), generated-parser-matches-grammar, govulncheck, lint); re-ran gh -R block/spirit pr checks 1134 directly and all are green.

Blocking

1. canRestoreBinaryChildWithoutParentheses drops parens for BETWEEN/IN/LIKE/MEMBER OF as if they were ordinary left-associative infix operators, but their right-hand sides are fixed multi-token productions, not simple operands. At expressions.go:1081, the side == binaryOpLeftSide branch fires for these four operators the same way it does for +/-/=, since restoreBinaryPrecedence gives them no operator-specific carve-out. A CHECK constraint shaped like CHECK ((a BETWEEN 1 AND 2) BETWEEN 3 AND 4) gets canonicalized to a BETWEEN 1 AND 2 BETWEEN 3 AND 4, which re-parses to a different tree (right-associated instead of the original nesting); the IN/LIKE/MEMBER OF variants restore to text that fails to re-parse at all, so applying that DDL to MySQL errors. No existing test nests one of these four constructs as another instance's subject operand.

Non-blocking

2. FromString's integer-digit clamp on decimal overflow doesn't adjust strIdx, so the digit-extraction loop reads trailing digits into the diagnostic value instead of leading ones. At mydecimal.go:241, digitsInt is clamped but the pre-clamp cursor captured at line 247 is left unchanged, so a 130-digit literal 9×129 + 1 produces a warning ending ...991 (the literal's tail) rather than MySQL's actual 65-nines value. The stored decimal itself is correct (overwritten by mysql.DefaultDecimal right after), and no caller currently reads the warnings slice, so this is a diagnostic-text-only bug.
3. pkg/statement/README.md's normalizer table still documents the pre-PR fully-parenthesized canonical form. README.md:346 says CHECK/generated-column expressions canonicalize to a fully-parenthesized form, but this PR's normalize_expression_parens.go change now produces minimal-paren output — the doc is stale as a direct side effect of this PR and should be updated alongside it.

The one thing that could have broken, verified

The riskiest mechanism is the new precedence-based paren-dropping rule for BETWEEN/IN/LIKE/MEMBER OF at expressions.go:1069-1082, since it's exercised on every CHECK-constraint/generated-column diff via pkg/statement's canonicalizer. I built a standalone probe against the checked-out worktree and ran restore-then-reparse round trips: ordinary chained operators and COLLATE ((a-b)-c, (a=b)=c, (a COLLATE x) COLLATE y) reparse to identical trees, confirming the rule is sound there, but (a BETWEEN 1 AND 2) BETWEEN 3 AND 4 reparses to a different tree, and the IN/LIKE/MEMBER OF analogues fail to reparse at all — confirming the rule is unsound for these four constructs (Finding 1).

Verified correct

• opensTransaction is correctly propagated from extractTablesFromDDLStmts to all three call sites, with binlog.go's two sites intentionally discarding it via _ (utils.go).
• gtid.go's pending-GTID-promotion gate correctly defers promotion for BEGIN/CTAS-with-implicit-commit while leaving standalone DDL and existing COMMIT/ROLLBACK/XA paths untouched (gtid.go:790).
• fixWordCntError's clamping keeps wordsInt+wordsFrac<=wordBufLen, so the >81-digit fix never writes out of bounds despite the corrupted warning text (Finding 2) (mydecimal.go).
• Top-level paren stripping (ctx.ParentBinaryOp==0) faithfully reproduces the prior manual unwrap it replaced (expressions.go:1028-1029).
• Unary-operand conservatism keeps parentheses wherever dropping them would be unsound (expressions.go:1014, :1020).
• Ordinary infix/COLLATE chains round-trip to identical trees (verified with a standalone probe, not just code reading).
• go build ./..., go test ./pkg/parser/..., and non-integration pkg/change/pkg/statement unit tests all pass locally.
• CI: 13/13 checks pass at head 7ccffd3 (DCO Check, build x7 matrix jobs, build(gtid), build(nogtid), generated-parser-matches-grammar, govulncheck, lint); re-ran gh -R block/spirit pr checks 1134 directly, all green.

This review was generated by Claude Code (claude-fable-5).

[morgo]

🤖 Thanks — findings 1 and 3 are fixed in 06c7597. Finding 1 was real and somewhat larger than reported. Finding 2 I believe is incorrect; evidence below.

1. Predicate-operator parens (blocking) — fixed, and broader than reported

Reproduced and then confirmed against live MySQL 8.0.46 and 9.7.0 in both directions: SELECT 0 BETWEEN 1 AND 10 BETWEEN 0 AND 0 returns 0, so the server really does regroup it right-associatively rather than reparse the original nesting, and the IN/LIKE/MEMBER OF analogues are ERROR 1064 outright. The fork's parser was faithful to MySQL here — the bug was entirely in the paren-dropping rule.

Probing the shape space found 12 broken forms, not 4, in two ways the report didn't cover:

• REGEXP is affected too — (s REGEXP 'x') REGEXP 'y' has the same failure.
• It isn't limited to same-precedence subjects. MySQL reads a = b BETWEEN 1 AND 10 as a = (b BETWEEN 1 AND 10), so a lower-precedence subject can't drop its parens either: (a = b) BETWEEN 1 AND 10, (a IS NULL) IN (1,2). The right-hand operands break the same way — a BETWEEN 1 AND (b = c), s LIKE (a = b).

Root cause is that these five aren't ordinary infix operators: MySQL fixes each operand to a specific grammar production rather than an expression at the operator's own level. The fix models that. The subject of all five is a bit_expr, so it may drop parentheses only when it binds at least as tightly as bitwise | — (a + b) IN (1,2) still minimizes to a + b IN (1,2), which is the point of the flag — and the other operands keep theirs, since they vary per operator (LIKE's pattern is a simple_expr, BETWEEN's bounds are a bit_expr and a predicate). Modelling each operand position individually would need new RestoreCtx plumbing for 3-operand BETWEEN; not worth it for text that is already correct, just not minimal.

Tests pin 19 fixed points (the 16 predicate shapes plus 3 that must still minimize) and 3 pairs that must stay textually distinct.

One extra gap found while checking this: REGEXP's pattern was a SimpleExpr in the fork's grammar, but MySQL's is a bit_expr — so 'a' REGEXP 'a' + 'b' was wrongly rejected. Fixed and pinned, along with the fact that the same shape after LIKE is an error, on the fork and on the server alike.

2. Decimal warning digits — not a bug; MySQL prints exactly this

The behaviour is real but it matches the server, so changing it would introduce divergence rather than remove it. Your own example, on both servers:

mysql> SELECT <129 nines then a 1>;
99999999999999999999999999999999999999999999999999999999999999999
Warning 1292 Truncated incorrect DECIMAL value: '999999999999999999999999999999999999999999999999999999999999999999999999999999991'

That warning ends ...991 — the literal's tail, 81 digits counted from the right. A less symmetric case makes it unambiguous: for 65 1s followed by 65 2s, MySQL warns with 16 ones + 65 twos, i.e. the last 81 digits, not the first. Identical on 8.0.46 and 9.7.0.

So the warning quotes a truncation of the literal (keeping trailing digits), not a rendering of the clamped value — the "MySQL's actual 65-nines value" in the finding is the query result, which the fork already returns correctly. I left the code alone, added a comment explaining why strIdx deliberately stays put, and added a subtest that pins the MySQL-matching output so a future reader doesn't "fix" it in the direction this finding suggests.

3. Stale README table — fixed

pkg/statement/README.md:346 now describes the minimal-paren canonical form, with both the before and after shapes.

[morgo]

🤖 Correction to my reply above: the first cut of the fix kept parentheses around every non-subject operand, which was too blunt — it regressed the ported a BETWEEN (b + 1) AND (c * 2) case, since BETWEEN's bounds are bit_exprs and minimize like any other. CI caught it; b72f6ac holds each operand position to its own production instead.

Subjects, BETWEEN's bounds and REGEXP's pattern are bit_exprs; LIKE's pattern and MEMBER OF's document are simple_exprs, which only COLLATE reaches. Each half checked on 9.7.0: SELECT 1 BETWEEN 0=0 AND 2 is a syntax error, SELECT 'A' LIKE 'a' COLLATE latin1_bin returns 0 (so COLLATE binds to the pattern, not to the LIKE), and SELECT 1 MEMBER OF (JSON_ARRAY(1)|0) is a syntax error. Six restore-level shapes added alongside the canonicalizer ones.