[fix](fe) Quote dictionary data load identifiers - #66594
Conversation
|
/review |
|
run feut |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for one correctness gap in the identifier fix.
Critical checkpoints:
- Correctness and name resolution: Per-part quoting safely round-trips the source and parser-level target, but the production dictionary-sink conversion re-splits a dot-joined target. Legal embedded-dot dictionary names still fail refresh before target binding.
- Security: Embedded backticks are doubled and restored correctly, and no additional statement-structure escape was found in source or target rendering.
- Compatibility, lifecycle, and performance: Valid persisted source qualifiers remain complete across create/replay; no additional compatibility, concurrency, lifecycle, error-path, or material performance issue was found.
- Tests: The new test is discoverable and validates parser escaping, but it stops before
InsertIntoDictionaryCommandand misses the accepted production-path failure. - User focus: No additional focus was provided; the full two-file PR scope was reviewed.
- Review completion: The review converged after two full rounds; every Round 2 full-coverage and risk-focused subagent returned
NO_NEW_VALUABLE_FINDINGS, and all candidates are accepted, deduplicated, or dismissed with evidence. Per the review-runner contract, no build was run.
| } | ||
|
|
||
| static String buildDataLoadSql(Dictionary dictionary) { | ||
| String targetName = Utils.qualifiedNameWithBackquote( |
There was a problem hiding this comment.
[P1] Preserve the structured target name past parsing
A dictionary name containing an embedded . is legal when the default enable_unicode_name_support=true, but this quoted two-part target is discarded by InsertIntoDictionaryCommand. UnboundDictionarySink rebuilds its name parts with dictionary.getNameWithFullQualifiers().split("\\."), so a name such as dict.with.dot becomes more than three parts; InsertIntoTableCommand.initPlan() then fails in RelationUtil.getQualifierName() before the dictionary-specific target override runs. The new test stops at the pre-replacement parser AST and does not cover an embedded-dot target, so this advertised identifier fix still leaves that dictionary unable to refresh. Please preserve dictionary.getFullQualifiers() as structured parts in the replacement sink and cover the production conversion/planning path with an embedded-dot target.
c2a93a6 to
9c5a41c
Compare
|
/review |
|
run feut |
There was a problem hiding this comment.
Requesting changes for one remaining catalog-state correctness gap.
Critical checkpoints:
- Correctness and catalog resolution: Per-part quoting safely round-trips target and source identifiers, and the current
getFullQualifiers()sink change resolves the existing dotted-name thread. The generated target is still only two parts, however, so initial sink classification inherits a manual caller's external current catalog; see the inline P1. - Lifecycle and error recovery: That parser/conversion failure occurs after the dictionary enters
LOADINGbut beforedataLoad()'s restoringtry, so manual refresh can strand the dictionary inLOADING. - Security, compatibility, and performance: Embedded backticks are doubled and restored correctly, the source remains an explicit three-part relation, and no additional injection, persisted-metadata, concurrency, configuration, or material performance issue was found.
- Tests: The unit test validates exact escaping and parsed/replacement name parts, but its internal-default context and AST-only stopping point miss the external-current-catalog failure and status consequence.
- User focus: No additional review focus was provided; the complete three-file PR scope was reviewed.
- Review completion: The review converged after two full rounds. Every Round 2 full-coverage and risk-focused agent returned
NO_NEW_VALUABLE_FINDINGS; all candidates are accepted, deduplicated, or dismissed with evidence. Per the review-runner contract, no build or test was run.
| } | ||
|
|
||
| static String buildDataLoadSql(Dictionary dictionary) { | ||
| String targetName = Utils.qualifiedNameWithBackquote( |
There was a problem hiding this comment.
[P1] Fully qualify the dictionary target before parsing
This still serializes the internal dictionary target as only db.dictionary. For a manual REFRESH DICTIONARY db.dictionary issued while an external catalog is current, visitInsertTable() resolves those two parts against that current catalog and creates an external sink (or errors for an unsupported catalog); InsertIntoDictionaryCommand then rejects it before the new dictionary-sink replacement can run. Because parsing/conversion also happen before dataLoad()'s status-restoring try, this failure can leave the dictionary stuck in LOADING. Please quote dictionary.getFullQualifiers() for the target as well, and cover a refresh path with a non-internal current catalog.
### What problem does this PR solve? 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. ### Release note Fix dictionary refresh for quoted object names. ### Check List (For Author) - 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
9c5a41c to
a4b3388
Compare
|
/review |
|
run feut |
There was a problem hiding this comment.
Review opinion: no additional changes are requested for the current head.
Critical checkpoints:
- Correctness and catalog resolution: Target and source names are serialized from structured qualifier lists with per-component backtick escaping. The explicit three-part internal target is classified independently of the session's current catalog, and the dictionary-sink replacement preserves those structured parts through command retries, reconstruction, collection, and binding.
- Lifecycle and concurrency: Context creation, SQL construction, parsing, command conversion, rule setup, and execution now share the status-recovery boundary. Manual and scheduled failures restore the captured
NORMAL/OUT_OF_DATEstate; query-state failures and the existing drop path remain covered. No distinct wrong-state or concurrency regression was found. - Security, compatibility, and performance: Dots, embedded backticks, spaces, and SQL-looking identifier text remain within their original components. Persisted metadata semantics are unchanged, and the small per-refresh quoting/list-copy cost is immaterial.
- Tests and CI: The new unit tests cover identifier escaping, parser sink selection under an external default catalog, dictionary-sink replacement, exact source/target name parts, and preparation-failure status recovery. Per the runner contract, no local build or test was run; CheckStyle passed and FE UT was still pending when this review was submitted.
- Existing review context: The current head resolves both earlier inline issues—the dotted-name split and the two-part target/status-recovery gap. No duplicate inline comment is being added.
- User focus: No additional focus was provided; the complete three-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 required rechecks, and the main-agent final sweep left no unresolved candidate. This is a comment-only review with zero inline comments.
1 participant
What problem does this PR solve?
Issue Number: DORIS-27640
Related PR: #66218
Problem Summary: Dictionary refresh rebuilt its internal
INSERT ... SELECTstatement 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.The dictionary sink conversion also rebuilt structured target qualifiers by joining and splitting on
., which broke legal database or dictionary names containing dots. Keep the structured qualifier list through that conversion.Fully qualify the internal dictionary target so manual refresh does not inherit an external current catalog. Command preparation now runs inside the existing status-recovery block, preventing preparation errors from leaving a dictionary in
LOADING.Release note
Fix dictionary refresh for quoted object names.
Check List (For Author)
protocbinary is unavailable in this worktree; official FE UT was triggered on the PR