Repository navigation
[improvement](parser) Add case-insensitive stream fast paths - #67452
Merged
morrySnow merged 4 commits intoSep 10, 2026
Merged
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 16942 ms |
Contributor
TPC-DS: Total hot run time: 81938 ms |
Contributor
ClickBench: Total hot run time: 14.52 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Add a reference-based character stream and lexer differential test plus JMH coverage for stream creation, case-folding lookahead, tokenization, and end-to-end parsing. This freezes a shared harness before evaluating P4 implementations.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- mvn -Pbenchmark -pl fe-sql-parser,fe-sql-parser-benchmark -Dmaven.build.cache.enabled=false package (170 tests passed)
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CaseInsensitiveStream called Character.toUpperCase for every lexer lookahead. Fold ASCII lowercase characters with arithmetic while preserving the Character.toUpperCase fallback for all other code points. Exploratory JMH reduced prebuilt lowercase folding from 2.041 to 1.570 us/op and uppercase folding from 2.044 to 1.598 us/op.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- mvn -pl fe-sql-parser -Dmaven.build.cache.enabled=false -Dtest=CaseInsensitiveStreamTest test
- Behavior changed: No
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: ANTLR copies every Java String into a code-point buffer before lexing. Read strings without surrogate code units directly while retaining the original ANTLR stream whenever code-point and UTF-16 indices differ. A 3-fork exploratory JMH run reduced identifier-heavy tokenization by 27.0%-28.2% and allocation by about 29%; short-query tokenization improved by 22.1%.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- mvn -pl fe-sql-parser -Dmaven.build.cache.enabled=false -Dtest=CaseInsensitiveStreamTest test
- ./run-fe-ut.sh --run org.apache.doris.httpv2.websql.SingleStatementValidatorTest,org.apache.doris.nereids.parser.NereidsParserTest,org.apache.doris.nereids.parser.NereidsParserDigestTest (91 tests passed)
- Behavior changed: No
- Does this need documentation: No
morrySnow
force-pushed
the
codex/antlr4-case-folding-fast-path
branch
from
September 8, 2026 07:26
7aa7b5e to
9253bbb
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
morrySnow
marked this pull request as ready for review
September 9, 2026 07:52
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
September 9, 2026 07:52
Contributor
TPC-H: Total hot run time: 16868 ms |
Contributor
TPC-DS: Total hot run time: 82319 ms |
Contributor
ClickBench: Total hot run time: 14.68 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
approved these changes
Sep 10, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
morningman
added a commit
to morningman/doris
that referenced
this pull request
Nov 13, 2026
…characters ### Background Several FE features rewrite the text of a statement by cutting the original SQL string at token positions from the parser. Audit masking replaces credential properties and passwords with `*XXX` before the statement reaches `fe.audit.log`, `__internal_schema.audit_log` and the FE log; CREATE / ALTER VIEW and CREATE MATERIALIZED VIEW expand `*` and qualify names in the definition they store; the parser reads `/*+ ... */` hints from comment tokens. The lexer counts its input by Unicode code point, while a Java `String` holds a supplementary character, such as an emoji, as two `char`s. ### What problem does this PR solve? Issue Number: None Related PR: apache#47595 (introduced the bug in audit masking), apache#32743 (introduced the bug in view rewriting), apache#70338 (its review found the masking leak) PR Type: Bugfix Problem Summary: After each supplementary character in a statement, these token positions point one `char` too early, so the text is cut at the wrong place. A credential after or inside such characters stays partly or fully in clear text in the audit log, CREATE VIEW fails with a syntax error, CREATE MATERIALIZED VIEW stores a broken query, and a hint after such a character is silently ignored. This PR converts the positions to `String` offsets, or reads the text through the token, at the five places that cut the original SQL. Tests are 89% of the diff. ### Details - **Change map:** production code +33 -33 in 3 files; tests +240 in 5 files (FE unit tests and one regression suite); generated `.out` +16. (1) `BaseViewInfo.rewriteSql`, shared by audit masking, views and MTMV, converts its positions; the sync-MV builder drops its private copy of it, and `rewriteProjectsToUserDefineAlias` replaces the select list through it. (2) `NereidsParser.getHintMap` reads hints from the token text. (3) `NereidsParser.tryParseExplainPlan` converts the start of the EXPLAIN PLAN body. - **Frequency:** Always, when a character above U+FFFF (an emoji, CJK Extension B) comes before or inside the rewritten part, for example a catalog COMMENT of emoji in front of `'s3.secret_key' = 'secret'`. - **Root cause:** `CaseInsensitiveStream.fromString` keeps ANTLR's code-point stream for any input with a surrogate (before apache#67452 every input used `CharStreams.fromString`), so `Token.getStartIndex()` and `getStopIndex()` count code points. The five places in the Change map used them as `String` indexes. - **Why existing tests missed it:** Unit tests: the masking (`EncryptSQLTest`, `RepositoryAuditEncryptionTest`), view and MTMV (`CreateViewTest`, `CreateMTMVCommandTest`) and hint tests (`LeanTokenModeTest`) use only BMP text, where both kinds of position coincide; `CaseInsensitiveStreamTest` uses an emoji but checks only the stream's own positions. Regression: `audit/test_external_catalog_credential_masking` and the `ddl_p0/create_view_nereids` suites have no supplementary characters, and the masking suite only checks that the whole credential is absent, which a leaked tail passes. - **Fix:** Convert where the text is cut. `rewriteSql` converts each position with `String.offsetByCodePoints`, walking the sorted positions once so the cost stays linear; `rewriteProjectsToUserDefineAlias` reads expressions through the token's input stream; `getHintMap` reads the hint from the token text, as `removeCommentAndTrimBlank` does. Changing the stream to report `String` positions was rejected: apache#67452 made code-point positions part of `CaseInsensitiveStream`'s contract, and `CaseInsensitiveStreamTest` pins them. - **Other paths and branches:** Fixed here: the 19 masking visitors of `LogicalPlanBuilderForEncryption` with every caller of `geneEncryptionSQL` (audit log, FE statement log, `_query_plan`) and the streaming insert job; the view, MTMV and sync-MV rewrites. Not affected: the other uses of token positions read text through the token's own stream (such as `getOriginSql`) or only compare positions with each other. branch-3.1, branch-4.0, branch-4.1 and branch-4.2 have the same code. Introduced by apache#32743 (views), apache#47595 (masking), apache#33699 (sync MV) and apache#33262 (EXPLAIN PLAN); hints since at least apache#39590. ### Testing - New unit tests fail on master `f1bc8b3690c` and pass with the fix, run locally: `EncryptSQLTest#testEncryptionAfterSupplementaryCharacters`, through `MysqlConnectProcessor` to the audit event (emoji inside a credential, in a catalog COMMENT, in the first of two property clauses, in a user name before `PASSWORD(...)`; master left `secret`, `abc`, `123` and `123456` in clear text); `CreateViewTest#testCreateViewRewritesAfterSupplementaryCharacters` (master: `mismatched input '`e`'`); `CreateMTMVCommandTest#testCreateMTMVRewritesQuerySqlAfterSupplementaryCharacters` (master kept ``SELECT '😀😀' A`e` e`internal`...``); `SupplementaryCharacterOffsetTest` (master dropped the hint, cut the EXPLAIN PLAN body to `AN SELECT 100`, and rewrote the sync MV to `avg_stateg(k)))`). - New regression suite `ddl_p0/create_view_nereids/test_create_view_supplementary_chars`: fails without the fix with the same syntax error and passes with it; the `.out` is generated by the script. Run locally on one FE and one BE, an older master FE build with the three changed classes swapped in. - 27 existing FE UT classes around the change (467 tests) pass. Reverting each part of the fix alone, moving the end of a replaced span by one, or not advancing the position cursor each fails a new test; the off-by-one also fails existing ASCII cases. - By hand on that cluster, without the fix: the secret stayed in `audit_log`, `/* 😀 */ select /*+ SET_VAR(query_timeout=1) */ sleep(3)` ran for 3 s, and an MTMV over `select '😀😀' as e, ...` was created but failed to refresh (`Nereids parse failed`). With the fix: `"s3.secret_key" = "*XXX")`, and the query times out after 1 s. ### Notes for Reviewers - `getHintMap` loses its `sql` parameter, which nothing reads any more; its three callers are in `NereidsParser`. - `rewriteProjectsToUserDefineAlias` replaces the select list through `rewriteSql` instead of `StringUtils.overlay`, so the conversion lives in one method. - Stored definitions are not repaired: an MTMV created by an affected version from such a query keeps its broken definition and has to be recreated. ### Release note Fix passwords and credentials left partly or fully unmasked in the audit log and the FE log when a supplementary character, such as an emoji, comes before or inside them. Also fix CREATE VIEW failing, CREATE MATERIALIZED VIEW storing a broken query, and hints such as SET_VAR being ignored after such a character; materialized views already created with a broken query must be recreated. ### Check List (For Author) - Behavior changed: No - Does this need documentation: No - Pre-submission status: Ready ### AI Disclosure - Tool: Claude Code (Claude Opus 5.5) - Generated: the fix, the tests, and this description - Human review: The author reviewed part of the changes. - Prompts: After the review bot on apache#70338 reported the masking leak, the author confirmed it is an existing engine defect shared by all masking paths and asked for a separate engine PR that fixes it, followed by a reply on apache#70338 with the link. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.
Already 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.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Every SQL lexer currently copies its input through
CharStreams.fromString()and callsCharacter.toUpperCase()for every case-insensitive lookahead. This is visible in lexer allocation and identifier-heavy parsing time.Add two strictly compatible fast paths:
a-zwith arithmetic and retainCharacter.toUpperCase()for every other code point.getText, navigation, and errors remain unchanged.The public arbitrary-
CharStreamconstructor is preserved. All production String entry points now use the factory. ANTLR's nativecaseInsensitiveoption was evaluated but rejected because it changed existing Unicode behavior such as the handling ofſelect.Benchmark
Environment and method:
cf33a08bcd5, artifact SHA-25651700bef953361218912abc6aec348cecdec3fe61031eba10cc1ebc534bc91837aa7b5e6487, artifact SHA-2566e8edf78befc0dc349be398aae454c172b5cb968ee98d88a0e056d4c21a9c5d0-prof gcselect 1The isolated
createLexermeasurement for the surrogate-containing string/comment workload regresses by 5.9% because the compatibility guard scans for surrogates before falling back. The corresponding complete tokenize and parse paths improve by 3.6% and 10.8%; no end-to-end control workload regressed. Prebuilt lowercase character folding improves by 10.6%.The performance gains come from eliminating the copied input buffer for BMP-only SQL, avoiding its allocation, and replacing the common lowercase ASCII
Character.toUpperCase()call with an arithmetic branch.Correctness corpus:
69c811d0d80c52b40d8cd854e925ff4930aa6601c7d1e66541f2eb29f35db50a.noBackslashEscapesmodes); complete token tuple and lexer-error signatures match. SHA-256:66780d4e1d9224ea27c8f715ab33edd247faed1571030ba9e90a8ef3f6212180.Release note
None
Check List (For Author)
Tested with:
mvn -Pbenchmark -pl fe-sql-parser,fe-sql-parser-benchmark -Dmaven.build.cache.enabled=false clean package: 170 tests passed; Checkstyle passed../run-fe-ut.sh --run org.apache.doris.httpv2.websql.SingleStatementValidatorTest,org.apache.doris.nereids.parser.NereidsParserTest,org.apache.doris.nereids.parser.NereidsParserDigestTest: 91 tests passed.Token and parse corpus comparisons described above: no differences.
FE-only
./build.sh --fewith a fresh output directory: passed and producedSuccessfully build Doris.Full
./run-fe-ut.shwas attempted, but the host's Surefire JVM stopped during the pre-existingfe-connector-trinoTrinoPredicateConverterTestdynamic-attach path before reachingfe-core; it was terminated after 90 minutes with no P4-related failure.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)