[fix](mtmv) support qualified materialized view rename target#63216
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one correctness issue in the qualified rename validation. The change otherwise stays focused on the MTMV rename parser/analyzer path, and the included unit test covers the happy path for a same-database qualified target.
Critical checkpoint conclusions:
- Goal/test: The PR aims to accept
ALTER MATERIALIZED VIEW db.mv RENAME db.new_mv; the positive same-db case is tested, but the cross-db rejection is incomplete for case-distinct DB names. - Scope: The code change is small and localized to parser/analyzer/test code.
- Concurrency/lifecycle: No new concurrency or special lifecycle management is introduced.
- Config/compatibility: No new config or storage/protocol compatibility impact.
- Parallel paths: This affects the Nereids MTMV alter path; no duplicate existing review threads were present.
- Tests: Positive unit coverage was added, but a negative case for
Testvstestis needed because internal DB lookup is case-sensitive. - Observability/performance/transactions: No new observability need or meaningful performance impact; rename persistence remains delegated to the existing
renameTablepath.
User focus: No additional user-provided review focus was supplied.
officialasishkumar
force-pushed
the
fix/mtmv-qualified-rename
branch
from
62505d5 to
5a0751a
Compare
|
Updated in Validation: |
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 31861 ms |
TPC-DS: Total hot run time: 168991 ms |
FE Regression Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Code review completed. I did not find additional blocking issues beyond the existing inline thread, which the current head appears to address by comparing catalog/db with exact equality and adding a case-distinct DB test.\n\nCritical checkpoint conclusions:\n- Goal/test: The PR allows qualified MTMV rename targets while preventing cross-database/catalog renames; the updated covers same-DB qualified rename and case-distinct DB rejection.\n- Scope: The change is small and focused on the MTMV rename grammar, plan builder, analyze logic, and focused test.\n- Concurrency/lifecycle: No new shared state, locks, threads, or non-obvious lifecycle/static initialization concerns were introduced.\n- Config/compatibility: No new configuration, storage format, edit log, or FE-BE protocol compatibility impact found.\n- Parallel paths: The renamed grammar labels keep REPLACE handling separate, and I did not find another MTMV rename path requiring the same change.\n- Error handling/data correctness: Cross-db/catalog rename is rejected during analysis; rename execution still uses the original database and target table name only. No transaction/data visibility concerns found.\n- Observability/performance: No new hot path or observability requirement identified.\n- Tests: Attempted [INFO] Scanning for projects...
[ERROR] [ERROR] Could not find the selected project in the reactor: fe-core @
[ERROR] Could not find the selected project in the reactor: fe-core -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MavenExecutionException, but it could not run in this runner because is missing.\n\nUser focus: No additional user-provided review focus was specified.
There was a problem hiding this comment.
Correction to my previous review summary: shell command substitution stripped parts of the test-command text in that comment. The review outcome is unchanged: I did not find additional blocking issues beyond the existing inline thread, and the current head appears to address that thread with exact catalog/db comparison plus a case-distinct DB test.
Critical checkpoint conclusions:
- Goal/test: The PR allows qualified MTMV rename targets while preventing cross-database/catalog renames; the updated AlterMTMVTest covers same-DB qualified rename and case-distinct DB rejection.
- Scope: The change is small and focused on the MTMV rename grammar, plan builder, analyze logic, and focused test.
- Concurrency/lifecycle: No new shared state, locks, threads, or non-obvious lifecycle/static initialization concerns were introduced.
- Config/compatibility: No new configuration, storage format, edit log, or FE-BE protocol compatibility impact found.
- Parallel paths: The renamed grammar labels keep REPLACE handling separate, and I did not find another MTMV rename path requiring the same change.
- Error handling/data correctness: Cross-db/catalog rename is rejected during analysis; rename execution still uses the original database and target table name only. No transaction/data visibility concerns found.
- Observability/performance: No new hot path or observability requirement identified.
- Tests: Attempted focused FE test with Maven, but it could not run in this runner because thirdparty/installed/bin/thrift is missing.
User focus: No additional user-provided review focus was specified.
FE Regression Coverage ReportIncrement line coverage |
4 participants
What problem does this PR solve?
Issue Number: close #62226
Related PR: None
Problem Summary:
ALTER MATERIALIZED VIEW db.mv RENAME db.new_mvfailed during parsing because the MTMVRENAMEtarget accepted only a single identifier. The underlying rename still operates inside the source database, so this PR parses the target as a multipart identifier and validates that any supplied catalog or database qualifier matches the source materialized view.Release note
Support qualified target names in
ALTER MATERIALIZED VIEW ... RENAMEwhen the qualifier matches the source materialized view.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)