tidb: match TiDB v8.5.5 block-comment lexing#372
Conversation
Container-verified rewrite of the comment lexer (pingcap/tidb:v8.5.5 oracle). Block comments do not nest in TiDB — the first */ closes — and the old nesting scan diverged in both directions: it swallowed valid SQL after pseudo-nested comments (GAP, including silent whole-statement loss) and accepted unclosed comments TiDB rejects (OVER). Unclosed /*! also panicked (slice bounds out of range) on the splice tail. - Regular comments end at the first */; an unclosed one is a parse error surfaced as a tokInvalid token (TiDB 1064 parity). - Bang comments (/*!, supported /*T!) splice their content as SQL. The closing */ is found with a string/backtick/line-comment/inner-comment aware scan (those constructs hide */ in TiDB because content is lexed as SQL), and an unclosed construct is tolerated exactly like TiDB's EOF-in-bang-mode. Inner bang markers are replaced with a space — inBangComment is a bool upstream, nested markers do not stack — and the closing */ splices to a space so butted tokens stay separate (x*/YZ lexes as x then YZ). - /*!NNNNN consumes exactly five version digits or none, mirroring scanVersionDigits(5,5); shorter or longer digit runs are SQL content. - /*T![...] feature lists follow scanFeatureIDs byte-for-byte: malformed lists reset and lex from '[' as bang content; well-formed lists with an unsupported ID downgrade to a regular comment. Feature map synced to v8.5.5 (adds force_inc, global_index, affinity — global_index content was previously dropped from the AST silently). - Split's skipBlockCommentMySQL is non-nesting, and a segment holding an unclosed comment reaches the lexer instead of being dropped as empty. Deliberate, documented leniencies (all reject-only on the TiDB side): TiDB's Lex lookahead corrupts inBangComment when AS/TO/MEMBER precedes the closing */ (upstream bug, not emulated); omni's lexer-wide unclosed string tolerance; SELECT * / 1 select-grammar acceptance. Verified: 520-probe generated sweep + 57-probe hand battery against the live v8.5.5 container — zero non-parked divergences, zero panics; adversarial splice audit confirms backtrack snapshots capture l.input, raw-text captures go through clamped inputText, and splicing stays idempotent.
AGREE_REJECT +3: the two unterminated-comment OVER clusters (1=1/*' and b = 'p' x2, select 1/*) now correctly reject. AGREE_ACCEPT -4 / GAP +4: feature-list sync (global_index, affinity) un-masks four wrong-AST acceptances — omni used to skip the whole /*T![...] comment and silently drop GLOBAL / AFFINITY = '...' from the AST; the content now injects correctly and rejects on the two known grammar gaps owned by wave-3 burn-down items (index-global-local-options, tidb-table-options-parity). Honest accounting, not a parser regression.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9417c1c37f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case ch == '#': | ||
| i = appendLineComment(&sb, l.input, i) |
There was a problem hiding this comment.
When this path lets a */ inside a bang-comment line comment be ignored, the top-level Split scanner still uses skipBlockCommentMySQL and resumes at that hidden */. If the bang content then contains a semicolon before the real close, Split cuts the input before the lexer can splice the whole comment; for example SELECT /*!50000 1 # */\n; SELECT 2 */; SELECT 3; is split into a second segment ending in SELECT 2 */ and is rejected, even though the new lexer semantics treat the hidden */ as part of the line comment. Please update Split's conditional-comment scan to use the same bang-aware closing rules.
Useful? React with 👍 / 👎.
1 participant
Wave 0 of the TiDB GAP burn-down (full-compat directive): container-verified rewrite of the block-comment lexer against the pinned
pingcap/tidb:v8.5.5oracle.What was wrong
TiDB block comments do not nest — the first
*/closes — and unclosed regular comments are parse errors. omni's nesting depth-scan diverged in both directions, plus a crash:/* a /* b */ SELECT 1— TiDB executes the SELECT; omni swallowed it as comment (parsed to 0 statements, invisible to every downstream consumer).SELECT 1 /* unterminatedaccepted; TiDB rejects 1064.SELECT /*! 1(unclosed/*!) →slice bounds out of rangein the splice tail./*T![global_index] GLOBAL */and/*T![affinity] ...content silently dropped — the v8.5 feature list was missingforce_inc,global_index,affinity.What changed
*/; unclosed →tokInvalidparse error (TiDB 1064 parity). Same non-nesting fix inSplit'sskipBlockCommentMySQL, and a segment holding an unclosed comment now reaches the lexer instead of being dropped as empty./*!, supported/*T!): content spliced as SQL. The closing*/is found with a string/backtick/line-comment/inner-comment-aware scan (those hide*/in TiDB since content is lexed as SQL); unclosed constructs are tolerated exactly like TiDB's EOF-in-bang-mode (fixes the panic). Inner bang markers and the closing*/splice to a single space so butted tokens stay separate (x*/YZlexes asxthenYZ; markers can't reconstruct phantom/*openers)./*!NNNNNconsumes exactly five version digits or none (scanVersionDigits(5,5)parity)./*T![...]feature lists followscanFeatureIDsbyte-for-byte: malformed lists reset and lex from[as content; well-formed lists with an unsupported ID downgrade to a regular comment. Feature map synced to v8.5.5.Deliberate parked leniencies (documented in
TestBangCommentParkedLeniencies, all reject-only on TiDB's side, fail-safe)Scanner.Lexlookahead forAS OF/TO TIMESTAMP/MEMBER OFcorruptsinBangCommentwhenAS/TO/MEMBERprecedes the closing*/(getNextTokenrestores the reader but not the flag) — an upstream bug we don't emulate.SELECT '1parses) — pre-existing, tracked separately.SELECT * / 1-class select-grammar acceptance — pre-existing, surfaces as trailing*/after a closed comment.Verification
l.input(splices are restore-safe), all raw-text captures go through clampedinputText, splicing is idempotent; the audit's one real finding (zero-byte marker strip could reconstruct a phantom/*) is fixed by the space-padding and regression-tested.go vet ./tidb/...clean;go test ./tidb/...green except the pre-existingtidb/catalognon-short container tests (collation-rendering mismatch, fails identically on main;-shortlane green).Scoreboard delta (adjudicated sweep, committed)
GLOBALindex option,AFFINITYtable option previously dropped silently). Honest accounting — the grammar gaps are owned by wave-3 burn-down items (index-global-local-options,tidb-table-options-parity).Mirror-back note: omni/mysql shares the fork-parent nesting comment scanner and the missing
||-era gaps do not apply here, but the non-nesting + unclosed-comment behavior is MySQL semantics too — candidate for the mysql lane (Jonathan), minus the TiDB-only/*T!machinery.🤖 Generated with Claude Code