[fix](fe) Quote dictionary data load identifiers - #67052
Open
Mryange wants to merge 1 commit into
Open
Conversation
Issue Number: None Related PR: apache#66218 Problem Summary: Dictionary refresh rebuilt its internal INSERT SELECT statement by concatenating raw target and source object names. Identifier text containing SQL syntax could therefore change the parsed query structure. Quote and escape every identifier component with the existing Nereids helper, and add a parser-level unit test that verifies hostile-looking names remain one source relation. Fix dictionary refresh for quoted object names. - Test: Unit test added; not run locally because the required thirdparty protoc binary is unavailable in the worktree - Behavior changed: Yes; dictionary load SQL now treats all metadata names strictly as identifiers - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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 opinion: no additional changes are requested for the current head.
Critical checkpoints:
- Goal, correctness, and proof: The change prevents dictionary metadata names from changing the internal
INSERT ... SELECTstructure. Each target/source component is quoted separately, embedded backticks are doubled and decoded back to the original identifier, and the explicit three-part target/source names remain independent of the caller's current catalog or database. The structural unit test checks the base sink, replacement dictionary sink, exactly one source relation, and exact decoded qualifier lists. - Scope and clarity: The production change is limited to one shared dictionary-load SQL builder plus the adjacent recovery boundary and one focused unit test. No parallel refresh path still concatenates raw dictionary/source names.
- Concurrency and lifecycle: The existing atomic
LOADINGtransition still excludes concurrent refreshes. Scheduled-context creation, SQL construction, parsing, command conversion, rule setup, and execution now share the existing status-recovery block; ordinary failures restore the capturedOUT_OF_DATEorNORMALstate. Retained database/dictionary identity continues to protect drop-and-recreate races, and no new lock-order or heavy-under-lock work is introduced. - Configuration, compatibility, and conditional paths: No configuration, persisted format, FE/BE protocol, rolling-upgrade contract, or transmitted variable changes are introduced. Scheduled, adaptive, create-triggered, and manual refreshes all converge on the same path. The existing conditional error-state, partial-load, abort, and commit behavior is unchanged.
- Tests and test results: The added parser/sink test covers dots, embedded backticks, whitespace, SQL-looking text, structured target/source parts, and an external default catalog. CheckStyle passed. The current checks do not show an FE compilation or execution of
DictionaryManagerTest, and per the review-runner contract no local build or test was run, so this review does not claim the new test passed. - Observability, persistence, transactions, and writes: Existing refresh logs, status/result reporting, EditLog version handling, commit/abort flow, and write atomicity are unchanged; no new metric or log is needed for this small serialization fix.
- Performance and security scope: The added work is linear in six short identifier components and occurs once per refresh. Under the repository threat model, the relevant FE SQL/parser and RBAC boundaries remain intact; identifier text cannot escape its quoted token or redirect the internal target/source binding.
- Other issues: No additional data-correctness, error-propagation, lifecycle, compatibility, performance, or coverage defect was substantiated.
- User focus: No additional user-provided focus was supplied; the complete two-file PR was reviewed.
- Review completion: The review converged in Round 1. Both normal full-scope reviewers and the separate risk-focused reviewer returned
NO_NEW_VALUABLE_FINDINGSafter their mandatory rechecks, and the main-agent final changed-file/unresolved-candidate sweep found zero accepted or unresolved candidates. This is a comment-only review with zero inline comments.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17031 ms |
Contributor
TPC-DS: Total hot run time: 81712 ms |
Contributor
ClickBench: Total hot run time: 14.54 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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
3 participants
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.
This change picks #66594 onto master and adapts it to the latest dictionary loading code. Dictionary refresh previously concatenated raw catalog, database, table, and dictionary names into an internal
INSERT ... SELECTstatement executed by the background loader. Identifier text containing SQL syntax could therefore alter the parsed query structure. The fix quotes and escapes every identifier component with the existing Nereids helper and adds a parser-level unit test to verify that hostile-looking names remain a single source relation.