yujun777 commented on code in PR #63216:
URL: https://github.com/apache/doris/pull/63216#discussion_r3238925426
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AlterMTMVRenameInfo.java:
##########
@@ -53,18 +53,25 @@ public AlterMTMVRenameInfo(TableNameInfo mvName, String
newName) {
public void analyze(ConnectContext ctx) throws AnalysisException {
super.analyze(ctx);
try {
- FeNameFormat.checkTableName(newName);
+ FeNameFormat.checkTableName(newName.getTbl());
} catch (org.apache.doris.common.AnalysisException e) {
throw new AnalysisException(e.getMessage(), e);
}
+ if (newName.getCtl() != null || newName.getDb() != null) {
+ newName.analyze(ctx.getNameSpaceContext());
+ if (!mvName.getCtl().equalsIgnoreCase(newName.getCtl())
Review Comment:
I do not think we should use `equalsIgnoreCase` to decide whether the target
catalog/db is the same as the source. This syntax only allows the rename target
to be written as a fully qualified name; it should not imply MV migration
support. If the target catalog/db is not exactly the same resolved object as
the source, this should fail explicitly. Otherwise a case-only difference or an
unnormalized catalog/db name may pass validation, while `run()` still executes
`renameTable(db, table, newName.getTbl())` in `mvName.getDb()` and never moves
the MV to the target db/catalog, which makes the SQL semantics misleading.
Please compare the resolved catalog/db names strictly and add negative tests
for different db/catalog targets.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AlterMTMVRenameInfo.java:
##########
@@ -53,18 +53,25 @@ public AlterMTMVRenameInfo(TableNameInfo mvName, String
newName) {
public void analyze(ConnectContext ctx) throws AnalysisException {
super.analyze(ctx);
try {
- FeNameFormat.checkTableName(newName);
+ FeNameFormat.checkTableName(newName.getTbl());
} catch (org.apache.doris.common.AnalysisException e) {
throw new AnalysisException(e.getMessage(), e);
}
+ if (newName.getCtl() != null || newName.getDb() != null) {
+ newName.analyze(ctx.getNameSpaceContext());
+ if (!mvName.getCtl().equalsIgnoreCase(newName.getCtl())
Review Comment:
Agree with this. This syntax should only mean that the rename target may be
written as a qualified name; it should not imply MV migration across databases
or catalogs. The target catalog/db should be validated as the same resolved
object as the source, otherwise the statement should fail because `run()` only
renames the table inside the source database.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/AlterMTMVRenameInfo.java:
##########
@@ -53,18 +53,25 @@ public AlterMTMVRenameInfo(TableNameInfo mvName, String
newName) {
public void analyze(ConnectContext ctx) throws AnalysisException {
super.analyze(ctx);
try {
- FeNameFormat.checkTableName(newName);
+ FeNameFormat.checkTableName(newName.getTbl());
} catch (org.apache.doris.common.AnalysisException e) {
throw new AnalysisException(e.getMessage(), e);
}
+ if (newName.getCtl() != null || newName.getDb() != null) {
+ newName.analyze(ctx.getNameSpaceContext());
+ if (!mvName.getCtl().equalsIgnoreCase(newName.getCtl())
Review Comment:
Agree with this. This syntax should only mean that the rename target may be
written as a qualified name; it should not imply MV migration across databases
or catalogs. The target catalog/db should be validated as the same resolved
object as the source, otherwise the statement should fail because `run()` only
renames the table inside the source database.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
