Skip to content

fix(parser): validate quoted Unicode identifiers#25879

Open
VioletQwQ-0 wants to merge 6 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/takeover-25793-unicode-aliases
Open

fix(parser): validate quoted Unicode identifiers#25879
VioletQwQ-0 wants to merge 6 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/takeover-25793-unicode-aliases

Conversation

@VioletQwQ-0

@VioletQwQ-0 VioletQwQ-0 commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

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 as CAST, CTEs, and CREATE 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:

  • keep valid Arabic and Chinese BMP identifiers accepted when backtick quoted;
  • reject invalid UTF-8, NUL, and supplementary-plane characters in quoted identifiers;
  • validate both the scanner fast path and escaped-delimiter slow path;
  • add scanner- and parser-level regression coverage.

Validation

  • go test -count=1 -run 'Test(QuotedUnicodeIdentifier|InvalidQuotedUnicodeIdentifiers)' ./pkg/sql/parsers/dialect/mysql
  • go test -race -count=10 -run 'Test(QuotedUnicodeIdentifier|InvalidQuotedUnicodeIdentifiers)' ./pkg/sql/parsers/dialect/mysql
  • go test -count=1 ./pkg/sql/parsers/...
  • go vet ./pkg/sql/parsers/...
  • git diff --check

@mergify

mergify Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

⚠️ The sha of the head commit of this PR conflicts with #25793. Mergify cannot evaluate rules on this PR. Once #25793 is merged or closed, Mergify will resume processing this PR. ⚠️

@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jul 20, 2026
@VioletQwQ-0 VioletQwQ-0 changed the title fix(parser): support Unicode select aliases fix(parser): validate quoted Unicode identifiers Jul 20, 2026
@VioletQwQ-0
VioletQwQ-0 marked this pull request as ready for review July 20, 2026 04:57
@VioletQwQ-0
VioletQwQ-0 requested a review from XuPeng-SH as a code owner July 20, 2026 04:57
@mergify mergify Bot added the kind/bug Something isn't working label Jul 20, 2026
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@matrix-meow matrix-meow added size/S Denotes a PR that changes [10,99] lines and removed size/M Denotes a PR that changes [100,499] lines labels Jul 20, 2026

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread pkg/sql/parsers/dialect/mysql/scanner.go Outdated

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Addressed the quoted user-variable regression in 6f2112a.

Changes:

  • schema/object/alias identifiers retain the BMP, NUL, and malformed UTF-8 restrictions;
  • single-@ quoted user variables use a separate validator that accepts valid utf8mb4 supplementary characters;
  • @@ system-variable handling remains unchanged;
  • added fast-path, escaped-backtick slow-path, malformed UTF-8, NUL, and parser-level regressions.

Validation:

  • go test -count=1 ./pkg/sql/parsers/dialect/mysql
  • go vet ./pkg/sql/parsers/dialect/mysql
  • go build ./pkg/sql/parsers/dialect/mysql
  • focused race tests passed 10 consecutive runs
  • git diff --check

@matrix-meow matrix-meow added size/M Denotes a PR that changes [100,499] lines and removed size/S Denotes a PR that changes [10,99] lines labels Jul 20, 2026

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Addressed the quoted-identifier hot-path regression in 23081459c.

Changes:

  • validate quoted names while scanning instead of decoding the completed identifier a second time;
  • scan ordinary ASCII eight bytes at a time while still detecting delimiters, NUL, newlines, and non-ASCII bytes before advancing;
  • decode only non-ASCII input, preserving the BMP-only policy for schema/object/alias identifiers and @@, while single-@ quoted user variables continue to accept valid utf8mb4 supplementary characters;
  • carry validation state through the escaped-delimiter slow path;
  • add chunk-boundary, fast/slow-path, ANSI_QUOTES, malformed UTF-8, NUL, supplementary-character, allocation, scanner benchmark, and ParseOne benchmark coverage.

Benchmark comparison on the same Apple M1 (8 x 500ms, medians, allocations unchanged):

  • old head -> this head: ASCII64 229.2 -> 29.1 ns/op, BMP64 557.7 -> 387.4 ns/op, escaped delimiter 356.2 -> 185.8 ns/op, ParseOne with three quoted identifiers 1784 -> 1533 ns/op;
  • clean main -> this head: ASCII64 97.8 -> 29.1 ns/op, escaped delimiter 193.7 -> 185.8 ns/op, and representative ParseOne 1490 -> 1533 ns/op (+2.9%, versus the prior roughly +20% direction on this machine).

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 0 alloc/op.

Validation:

  • go test -count=1 ./pkg/sql/parsers/dialect/mysql
  • focused quoted-identifier tests under -race
  • go vet ./pkg/sql/parsers/dialect/mysql
  • go build ./pkg/sql/parsers/dialect/mysql
  • git diff --check

Self-review covered ordinary/boundary inputs, fast and escaped paths, ANSI_QUOTES, single-@ versus @@, malformed UTF-8, line tracking, premature EOF, allocations, and Q1-Q3. No remaining blocker was found.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants