Repository navigation
[fix](nereids) Fix SQL masking and view rewrites after supplementary characters - #70366
Open
morningman wants to merge 1 commit into
Open
morningman wants to merge 1 commit into
morningman wants to merge 1 commit into
Conversation
morningman
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
November 7, 2026 08:36
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review completed at PR head 910dabb against base f1bc8b3. I found no verified blocking or non-blocking defects, so there are no new inline findings and no existing P0/P1 threads to carry forward.
The change goal is met: the shared rewriteSql path converts ANTLR code-point positions to Java UTF-16 offsets while preserving ordered multi-range and zero-width replacements; all audited masking, view/MTMV, sync-MV, and streaming callers use positions from the same source text. The alias rewrite reparses the already rewritten SQL and extracts exact source intervals, and the hint and EXPLAIN paths preserve token text and convert body offsets consistently.
Critical checkpoints: data/version/delete-bitmap, FE-BE protocol, persistence, transactions, concurrency, locking, lifecycle, configuration, observability, and BE memory/nullability concerns are not involved in this FE parser/string-only change. The focused FE/Nereids checks found no exception, rewrite-loop, or parallel-path regression. The added unit and regression tests cover supplementary characters in masking, views, MTMV, hints, EXPLAIN, aliases, and sync-MV rewriting; they were inspected but not executed because this invocation is static-only. No user-provided additional focus was supplied.
The review converged in round 1 after complete changed-file and call-chain coverage plus independent risk-focused reviews. Remaining limitation: runtime/build/test behavior was not executed under the review contract.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28118 ms |
Contributor
TPC-DS: Total hot run time: 153719 ms |
Contributor
ClickBench: Total hot run time: 23.91 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #70366 at head 910dabb against base f1bc8b3: no verified new defects found. All nine changed files and their relevant producer/consumer paths were reviewed. There are no existing inline findings to carry forward and no additional user-provided focus.
Critical checkpoint conclusions:
- Goal/correctness: the inspected code consistently converts ANTLR code-point positions at Java String boundaries. Masking, CREATE/ALTER VIEW, MTMV, sync-MV, hints and EXPLAIN use coordinates from the SQL being rewritten. Added tests assert the intended corrected text and hint objects.
- Scope/reuse: the three production changes are focused; sync-MV and explicit-column rewriting reuse BaseViewInfo.rewriteSql instead of separate slicing logic.
- Concurrency/locking: no new threads, shared mutable state or lock operations. Replacement maps, cursors and parser inputs remain local to the existing statement flow.
- Lifecycle/static initialization: no new resource ownership or static initialization. Token input remains reachable while its source text is extracted.
- Configuration: no configuration added or changed.
- Compatibility: getHintMap callers are updated; no wire or storage schema changes. Corrected definitions enter existing storage paths; already malformed stored MV definitions are not repaired by this change.
- Thrift/protobuf: no definitions changed; field-id review is inapplicable.
- Parallel paths: traced all direct shared-rewrite callers, including streaming job/task audit SQL, view/MTMV analysis-generated spans, generic/create-view/sync-MV hint parsing, and full-scan versus lean tokens. Encryption continues to omit execution hints intentionally.
- Conditional checks/errors: the hint prefix check matches the existing comment-normalization convention. Inclusive ends, BMP text, supplementary characters before and within replacements, adjacent ranges and query-relative sync-MV positions are consistent; existing exception/fallback behavior is preserved.
- Test coverage: inspected added FE tests for audit-event masking, views and explicit aliases, MTMV definitions, hints, EXPLAIN and sync-MV aggregates, plus the view regression suite. Existing BMP/control cases support compatibility. No new malformed-input semantics or error path is introduced.
- Expected results: the new regression definitions and rows match the suite's inserted data, filter and aliases; query results use order_qt. Output generation and author-reported test success were not independently executed.
- Observability: this local deterministic text correction adds no operational boundary requiring new logs or metrics; existing parser/MV/audit diagnostics remain available.
- Persistence/transactions: rewritten view/MV text uses existing persistence paths; no EditLog, transaction, failover or replay protocol changes.
- Data writes/atomicity: no change to data mutation, visibility, version or delete-bitmap logic; no new crash-recovery or write lifecycle.
- FE/BE variables: no new transmitted variables or send paths.
- Performance/memory: conversion advances through original input once for disjoint ranges and remains linear in source plus output length. Source extraction is bounded by the relevant intervals. No BE allocation or nullability code is involved.
- Other issues: no additional verified issue after independent review and the final missed-case sweep. The map comparator's existing end ordering and overlap limitations introduce no new failure for the inspected producers.
The review converged in two rounds after complete static coverage and independent challenge of this frozen payload. Both recorded coordinate/source-text risk focuses were dismissed with code evidence; no unresolved candidates or static coverage gaps remain. Validation limit: no builds, tests, benchmarks, cluster execution or output regeneration were performed under the static-only review contract.
Review converged after 2 round(s).
…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 <noreply@anthropic.com>
morningman
force-pushed
the
b2m-t38f-codepoint-offsets
branch
from
November 13, 2026 08:32
910dabb to
9f6f906
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of PR #70366 at head 9f6f906: no verified new defects found. All nine changed files and their relevant producer/consumer paths were reviewed. There are no existing inline P0/P1 findings to carry forward and no additional user-provided review focus.
Critical checkpoint conclusions:
- Goal and correctness: the changes consistently translate ANTLR code-point positions at Java String boundaries. Masking, view/MV definitions, hints and EXPLAIN use the text and coordinate origins expected by their callers. The added assertions exercise the intended corrected statements, definitions and hint objects.
- Scope and reuse: the fix is focused in three production files. Sync-MV and explicit-column rewriting reuse the shared replacement helper, removing duplicate slicing logic.
- Concurrency and locks: no threads, shared mutable state or lock operations are added. New cursors/builders are local to existing statement parsing and analysis.
- Lifecycle and initialization: token input remains reachable during extraction; no new retained resource, ownership rule or static initialization dependency.
- Configuration: no configuration changes.
- Compatibility: all in-repository getHintMap callers use the updated signature; no wire or storage-schema changes. Corrected view/MV definitions use existing persistence. Previously malformed stored MV definitions are not repaired automatically, as the PR documents.
- Thrift/protobuf: no definitions changed, so field-ID compatibility checks are inapplicable.
- Parallel paths: traced CREATE/ALTER VIEW, async/IVM and sync MV, audit logging, both FE statement-log masking paths, HTTP plan audit and streaming job/task rewriting. Generic, create-view and sync-MV hint parsing all use the updated method; masking continues to omit execution hints.
- Conditions and errors: bracketed comments survive lean tokenization with original text, and the prefix check matches the existing comment-normalization convention. Inclusive interval ends, query-relative sync-MV positions, empty/adjacent replacements and source-text identity are consistent. Existing failure boundaries remain intact.
- Test coverage: inspected new FE cases for supplementary characters before and inside credentials, multiple property regions, explicit view aliases, MTMV definitions, hints, EXPLAIN and sync-MV aggregate rewriting, alongside existing BMP/lean-token controls. No new malformed-input contract or failure path requires a different negative expectation.
- Expected results: regression SHOW CREATE definitions and selected rows match the inserted data, filter and aliases; row comparisons are ordered. Script generation and author-reported test success were not independently rerun.
- Observability: the local text-index correction introduces no new operational boundary requiring logs or metrics; existing parser, audit and MV diagnostics remain available.
- Persistence and transactions: no EditLog, replay, failover or transaction protocol changes. Rewritten definitions continue through existing validation/storage paths.
- Data writes and atomicity: row mutation, visibility/version, delete-bitmap and crash-recovery behavior are unchanged.
- FE/BE variables: no new transmitted variables or send paths.
- Performance and memory: the replacement cursor advances through the original disjoint spans once, retaining linear source/output complexity. No repeated prefix scan, new parser pass or BE allocation/nullability change was introduced.
- Additional review: all analysis-stage interval writers preserve parser-origin coordinates. The existing comparator orders by interval end, which also orders disjoint intervals by start; this change adds no overlapping-span producer. No further verified issue emerged from the independent reviews and final missed-case sweep.
The review converged in round 1: both complete-review agents and the additional risk-focused agent returned NO_NEW_VALUABLE_FINDINGS, all three recorded risks were resolved with code evidence, and no unresolved candidates or static coverage gaps remain. Validation limits: no builds, tests, benchmarks, cluster execution or output regeneration were performed under the static-only contract.
Review converged after 1 round(s).
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.
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
*XXXbefore the statement reachesfe.audit.log,__internal_schema.audit_logand 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 JavaStringholds a supplementary character, such as an emoji, as twochars.What problem does this PR solve?
Issue Number: None
Related PR: #47595 (introduced the bug in audit masking), #32743 (introduced the bug in view rewriting), #70338 (its review found the masking leak)
PR Type: Bugfix
Problem Summary:
After each supplementary character in a statement, these token positions point one
chartoo 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 toStringoffsets, or reads the text through the token, at the five places that cut the original SQL. Tests are 89% of the diff.Details
.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, andrewriteProjectsToUserDefineAliasreplaces the select list through it. (2)NereidsParser.getHintMapreads hints from the token text. (3)NereidsParser.tryParseExplainPlanconverts the start of the EXPLAIN PLAN body.'s3.secret_key' = 'secret'.CaseInsensitiveStream.fromStringkeeps ANTLR's code-point stream for any input with a surrogate (before [improvement](parser) Add case-insensitive stream fast paths #67452 every input usedCharStreams.fromString), soToken.getStartIndex()andgetStopIndex()count code points. The five places in the Change map used them asStringindexes.EncryptSQLTest,RepositoryAuditEncryptionTest), view and MTMV (CreateViewTest,CreateMTMVCommandTest) and hint tests (LeanTokenModeTest) use only BMP text, where both kinds of position coincide;CaseInsensitiveStreamTestuses an emoji but checks only the stream's own positions. Regression:audit/test_external_catalog_credential_maskingand theddl_p0/create_view_nereidssuites have no supplementary characters, and the masking suite only checks that the whole credential is absent, which a leaked tail passes.rewriteSqlconverts each position withString.offsetByCodePoints, walking the sorted positions once so the cost stays linear;rewriteProjectsToUserDefineAliasreads expressions through the token's input stream;getHintMapreads the hint from the token text, asremoveCommentAndTrimBlankdoes. Changing the stream to reportStringpositions was rejected: [improvement](parser) Add case-insensitive stream fast paths #67452 made code-point positions part ofCaseInsensitiveStream's contract, andCaseInsensitiveStreamTestpins them.LogicalPlanBuilderForEncryptionwith every caller ofgeneEncryptionSQL(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 asgetOriginSql) 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 [feat](nereids) support create view in nereids #32743 (views), [Feature](nereids) support encrypt sql for audit log #47595 (masking), [feature](nereids)use mtmv to match legacy mv #33699 (sync MV) and [enhancement](Nereids) Enable parse sql from sql cache #33262 (EXPLAIN PLAN); hints since at least [fix](Nereids) fix lexer Backtracking or Ambiguity cause of key word duplicate #39590.Testing
f1bc8b3690cand pass with the fix, run locally:EncryptSQLTest#testEncryptionAfterSupplementaryCharacters, throughMysqlConnectProcessorto the audit event (emoji inside a credential, in a catalog COMMENT, in the first of two property clauses, in a user name beforePASSWORD(...); master leftsecret,abc,123and123456in clear text);CreateViewTest#testCreateViewRewritesAfterSupplementaryCharacters(master:mismatched input 'e');CreateMTMVCommandTest#testCreateMTMVRewritesQuerySqlAfterSupplementaryCharacters(master keptSELECT '😀😀' A`e` e`internal`...);SupplementaryCharacterOffsetTest(master dropped the hint, cut the EXPLAIN PLAN body toAN SELECT 100, and rewrote the sync MV toavg_stateg(k)))).ddl_p0/create_view_nereids/test_create_view_supplementary_chars: fails without the fix with the same syntax error and passes with it; the.outis generated by the script. Run locally on one FE and one BE, an older master FE build with the three changed classes swapped in.audit_log,/* 😀 */ select /*+ SET_VAR(query_timeout=1) */ sleep(3)ran for 3 s, and an MTMV overselect '😀😀' 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
getHintMaploses itssqlparameter, which nothing reads any more; its three callers are inNereidsParser.rewriteProjectsToUserDefineAliasreplaces the select list throughrewriteSqlinstead ofStringUtils.overlay, so the conversion lives in one method.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)
AI Disclosure