[fix](fe) Reject sync MV creation when MV qualifier differs from base table qualifier#63352
Open
morrySnow wants to merge 3 commits into
Open
[fix](fe) Reject sync MV creation when MV qualifier differs from base table qualifier#63352morrySnow wants to merge 3 commits into
morrySnow wants to merge 3 commits into
Conversation
…table database ### What problem does this PR solve? Issue Number: close #DORIS-19133 Problem Summary: When creating a sync materialized view, if the MV name specified a different database than the base table (e.g. `CREATE MATERIALIZED VIEW db1.mv_t AS SELECT d FROM db.t`), Doris silently succeeded instead of raising an error. A sync materialized view is stored as an index on its base table and must therefore reside in the same database. The same error now also fires when the current session database differs from the base table's database. ### Release note Sync materialized view creation now fails with an explicit error when the materialized view's effective database (explicitly specified or current session database) does not match the base table's database. ### Check List (For Author) - Test: Regression test added under regression-test/suites/mv_p0/test_cross_db_mv_error/ - Behavior changed: Yes — previously the statement succeeded silently; now it raises an AnalysisException - Does this need documentation: No Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Refactor the database-consistency check into a package-private helper method checkDatabaseConsistency() so it can be tested directly without running the full Nereids planner. Add CreateMaterializedViewCommandTest with four unit tests covering: - same database: no exception - different database: AnalysisException with descriptive message - null mvDb: check skipped (no exception) - empty mvDb: check skipped (no exception) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found one blocking correctness gap in the new sync MV database validation. The PR fixes db-name mismatches but still ignores the MV catalog qualifier/current catalog, so catalog-mismatched MV names can be silently accepted and routed to the internal base table.
Critical checkpoint conclusions:
- Goal/test: The PR targets rejecting sync MV creation when the MV effective database differs from the base table database, and adds regression coverage for explicit/implicit database mismatches. Catalog mismatch coverage is missing.
- Scope: The implementation is small and focused, but incomplete for fully-qualified MV names because TableNameInfo carries catalog as well as database.
- Concurrency/lifecycle: No new concurrency or lifecycle-sensitive state is introduced.
- Configuration/compatibility: No new config, protocol, storage, or persistence compatibility changes.
- Parallel paths: Sync MV creation routes through this command and downstream Alter handling; downstream still uses the base table db and only the MV table name, so the catalog/database validation must be complete before that point.
- Tests: Regression tests cover db mismatch and success in the same db, but not catalog-qualified MV names or current-catalog mismatch.
- Observability/performance: No additional observability needed; no performance concern in this validation path.
User focus: No additional user-provided review focus was present.
Contributor
FE UT Coverage ReportIncrement line coverage |
Extend the database/catalog validation to also reject sync MV creation when the MV's effective catalog is not the internal catalog. A sync materialized view is stored as an index on an OlapTable which always lives in the internal catalog. If the MV name carries an external catalog qualifier (e.g. hive_catalog.db.mv_t), or the current session catalog is not the internal catalog, creation must fail with an explicit AnalysisException. Changes: - Add checkCatalogConsistency() alongside checkDatabaseConsistency() and call it from validate() - Extend CreateMaterializedViewCommandTest with catalog check tests - Extend regression test with catalog mismatch case Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Fixed in the latest commit. Added |
morrySnow
changed the title
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31213 ms |
Contributor
TPC-DS: Total hot run time: 170793 ms |
Contributor
There was a problem hiding this comment.
No new blocking issues found in this pass.
Critical checkpoint conclusions:
- Goal and coverage: the PR rejects sync MV creation when the effective MV catalog/database does not match the internal base-table namespace; the regression test covers explicit cross-db, implicit session-db, explicit external-catalog rejection, and a same-db success case, with focused unit coverage for the helper checks.
- Scope: the implementation is small and localized to
CreateMaterializedViewCommandplus targeted tests. - Concurrency/lifecycle: no new concurrency, locking, persistence, or lifecycle-sensitive state is introduced.
- Compatibility: this is an intentional validation behavior change for invalid sync MV statements; no storage or wire-format compatibility concerns found.
- Parallel paths: sync MV creation flows through this command and downstream
Alter.processCreateMaterializedView()uses the base table database, so validating before creation is the right path. - Error handling: uses Nereids
AnalysisException, consistent with surrounding validation failures in this class. - Test results: I reviewed the added test logic but did not run the regression or FE unit tests in this runner.
- Observability/performance: no new hot path or observability requirement identified.
Existing review context: I did not repeat the already-known catalog-validation thread; the current head includes catalog consistency validation and an explicit catalog mismatch regression case.
User focus: no additional user-provided review focus was specified.
Contributor
TPC-H: Total hot run time: 31491 ms |
Contributor
TPC-DS: Total hot run time: 168802 ms |
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
2 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.
What problem does this PR solve?
Problem Summary: When creating a sync materialized view, if the MV name specified a different database than the base table (e.g.
CREATE MATERIALIZED VIEW db1.mv_t AS SELECT d FROM db.t), Doris silently succeeded instead of raising an error. A sync materialized view is stored as an index on its base table and must therefore reside in the same database. The same error now also fires when the current session database differs from the base table's database.Root cause
In
CreateMaterializedViewCommand.validate(), thedbNamefield is extracted from the base table (via the plan validator), but the MV name's database (name.getDb()) was never compared against it. As a result, specifying a different database in the MV name was silently ignored and the MV was always created under the base table's database without any error.Fix
After extracting
dbNamefrom the validator, determine the MV's effective database:name.getDb()if explicitly specified.If the effective MV database differs from the base table's database, throw an
AnalysisException.Release note
Sync materialized view creation now fails with an explicit error when the materialized view's effective database (explicitly specified or current session database) does not match the base table's database.
Check List (For Author)
regression-test/suites/mv_p0/test_cross_db_mv_error/