fix(parser): validate quoted Unicode identifiers#25879
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
There was a problem hiding this comment.
Requesting changes for one correctness regression. scanLiteralIdentifier is also used for backtick-quoted user-variable names after @. User variables are not schema identifiers, so the new BMP-only predicate changes pre-existing valid parser behavior: clean base accepts SELECT @`😀`, while this head returns a syntax error. MySQL documents that quoted user-variable names may contain otherwise-unavailable characters, and its lexer consumes that branch using the client multibyte character set rather than applying the schema-identifier BMP restriction. Please make validation context-aware: schema/object/alias identifiers may reject invalid UTF-8, NUL, and code points above U+FFFF, but quoted user variables must preserve valid utf8mb4 names while still rejecting malformed input as appropriate. Add regressions for SELECT @`😀` and the escaped-delimiter slow path. The rest of the review is clean: build, vet, full parser tests, and race tests pass; the validator adds no allocations and there are no resource/lifecycle risks. Also note that the exact quoted Arabic/Chinese cases from #25014 already pass on the clean base, so the actual delta here is validation hardening rather than enabling those valid cases.
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The shared validation introduces a parser compatibility regression for quoted user variables.
P1 - Do not apply the BMP restriction to quoted user variables (pkg/sql/parsers/dialect/mysql/scanner.go:580)
Scan routes @ followed by a backtick through scanLiteralIdentifier, then emits AT_ID; the grammar accepts that as a user variable. On origin/main, SELECT @😀`` was accepted because quoted contents were returned unchanged. This change rejects it via r > '\uFFFF', producing a syntax error. Escaped-backtick names use the same validator. Apply the restriction only to schema/object identifiers and add fast- and slow-path user-variable regressions.
|
Addressed the quoted user-variable regression in 6f2112a. Changes:
Validation:
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for a normal-path parser performance regression.
The correctness fix is now context-correct: schema/object/alias identifiers keep the MySQL BMP/NUL/malformed-UTF-8 restrictions, single-@ quoted user variables preserve valid utf8mb4 supplementary names, @@ remains strict, and both fast and escaped-delimiter paths are covered.
However, every quoted identifier is currently scanned twice: scanLiteralIdentifierWithValidator first walks every byte to find the closing delimiter, then validQuotedName decodes the complete string again. This affects ordinary ORM/generated SQL, where every database/table/column name may be backtick-quoted.
Reproducible microbenchmarks on the clean base versus this head (Ryzen 9 7900X, 8 runs, 500 ms, unchanged allocations):
- 64-byte ASCII quoted identifier: median ~76.6 ns -> ~114.8 ns (+50%)
- 64-rune BMP quoted identifier: median ~200.7 ns -> ~328 ns (+63%)
- escaped-delimiter identifier: median ~160 ns -> ~221 ns (+38%)
A ParseOne benchmark also shows the same direction: a short statement with three quoted ASCII identifiers moved from roughly 1.63 us to 2.02 us median (~24%, noisier than the scanner benchmark).
Please validate during the existing delimiter scan instead of doing a full second pass: track invalid/NUL state while advancing ASCII bytes and decode only when a non-ASCII byte is encountered (while retaining the separate supplementary policy for schema identifiers versus single-@ user variables). The escaped slow path can carry the same state or use its existing builder result because it is uncommon. A focused benchmark would prevent this from returning.
All correctness, malformed-input, ANSI_QUOTES, replacement-rune, fast/slow-path, parser-package, vet, and race checks otherwise pass; there are no resource/lifecycle concerns. Also, the exact quoted Arabic/Chinese cases from #25014 already pass on the clean base, so this PR's actual delta is validation hardening and should not materially slow the established valid path.
|
Addressed the quoted-identifier hot-path regression in Changes:
Benchmark comparison on the same Apple M1 (
BMP input still performs the required UTF-8/BMP validation that clean main does not; the normal quoted-ASCII path no longer performs a second scan and remains Validation:
Self-review covered ordinary/boundary inputs, fast and escaped paths, ANSI_QUOTES, single- |
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
No correctness, compatibility, lifecycle, concurrency, performance, or test-coverage regressions found. Verified with package tests, focused race tests, vet, and diff check.
What type of PR is this?
Which issue(s) this PR fixes:
issue #25014
What this PR does / why we need it:
This PR continues the unfinished work from #25793 while preserving its original commits and authorship.
The review found that the inherited lexer heuristic enabled Unicode identifiers after every
AS, not only for select aliases. That changed tokenization and error columns in unrelated syntax such asCAST, CTEs, andCREATE TABLE ... AS. The heuristic and its out-of-scope unquoted-identifier changes have therefore been removed.The remaining change is scoped to the issue's quoted-identifier requirement:
Validation
go test -count=1 -run 'Test(QuotedUnicodeIdentifier|InvalidQuotedUnicodeIdentifiers)' ./pkg/sql/parsers/dialect/mysqlgo test -race -count=10 -run 'Test(QuotedUnicodeIdentifier|InvalidQuotedUnicodeIdentifiers)' ./pkg/sql/parsers/dialect/mysqlgo test -count=1 ./pkg/sql/parsers/...go vet ./pkg/sql/parsers/...git diff --check