morrySnow commented on code in PR #65539:
URL: https://github.com/apache/doris/pull/65539#discussion_r3575773680
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -1329,6 +1335,10 @@ public void processAlterMTMV(AlterMTMV alterMTMV,
boolean isReplay) {
if (alterMTMV.isNeedRebuildJob()) {
Env.getCurrentEnv().getMtmvService().alterJob(mtmv, isReplay);
}
+ // keep FE-side plan/cache consistent after MTMV schema change
+ if (alterSuccess && alterMTMV.getOpType() ==
MTMVAlterOpType.ALTER_ADD_COLUMN) {
+ Env.getCurrentEnv().notifyTableMetaChange(mtmv);
Review Comment:
**CRITICAL — Compilation error**:
`Env.getCurrentEnv().notifyTableMetaChange(mtmv)` calls a method that does
**not exist** anywhere in the codebase. A full-project grep for
`notifyTableMetaChange` returns zero results.
This code will not compile. The intended behavior (per the comment "keep
FE-side plan/cache consistent after MTMV schema change") should be achieved by:
1. Removing this call entirely, AND
2. Adding `this.rewriteCacheGeneration++` and clearing
`cacheWithGuard`/`cacheWithoutGuard` inside `MTMV.alterAddColumn()` itself —
following the pattern already used at `MTMV.java:496-498` in
`processBaseViewChange()`.
Alternatively, if a new `Env` method is truly needed, it must be added to
`Env.java` with the appropriate implementation.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -197,6 +206,80 @@ public MTMVStatus alterStatus(MTMVStatus newStatus) {
}
}
+ public void alterAddColumn(Column newColumn, String exprSql, String
newQuerySql) {
+ writeMvLock();
+ try {
+ querySql = StringUtils.isBlank(newQuerySql)
+ ? rewriteQuerySqlForAddColumn(querySql, exprSql,
newColumn.getName())
+ : newQuerySql;
+
+ MaterializedIndexMeta baseIndexMeta =
getIndexMetaByIndexId(getBaseIndexId());
+ List<Column> newSchema = new
ArrayList<>(baseIndexMeta.getSchema());
+ Column copied = new Column(newColumn);
+ copied.setUniqueId(baseIndexMeta.incAndGetMaxColUniqueId());
+ newSchema.add(copied);
+ baseIndexMeta.setSchema(newSchema);
+ baseIndexMeta.setSchemaVersion(baseIndexMeta.getSchemaVersion() +
1);
+ rebuildFullSchema();
Review Comment:
**BUG — Missing rewrite cache invalidation**: `alterAddColumn()` increments
`schemaChangeVersion` and resets `refreshSnapshot`, but does **not** increment
`rewriteCacheGeneration` or clear `cacheWithGuard` / `cacheWithoutGuard`.
Between the ALTER ADD COLUMN and the next successful refresh, the MTMV's
rewrite caches will serve stale results to query rewriting — matching incoming
queries against the old schema and potentially returning incorrect results.
The existing pattern in `processBaseViewChange()` (lines 496-498) already
shows the correct approach:
```java
rewriteCacheGeneration++;
cacheWithGuard = null;
cacheWithoutGuard = null;
```
Add these three lines here alongside `schemaChangeVersion++`.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -197,6 +206,80 @@ public MTMVStatus alterStatus(MTMVStatus newStatus) {
}
}
+ public void alterAddColumn(Column newColumn, String exprSql, String
newQuerySql) {
+ writeMvLock();
+ try {
+ querySql = StringUtils.isBlank(newQuerySql)
+ ? rewriteQuerySqlForAddColumn(querySql, exprSql,
newColumn.getName())
+ : newQuerySql;
+
+ MaterializedIndexMeta baseIndexMeta =
getIndexMetaByIndexId(getBaseIndexId());
+ List<Column> newSchema = new
ArrayList<>(baseIndexMeta.getSchema());
+ Column copied = new Column(newColumn);
+ copied.setUniqueId(baseIndexMeta.incAndGetMaxColUniqueId());
+ newSchema.add(copied);
+ baseIndexMeta.setSchema(newSchema);
+ baseIndexMeta.setSchemaVersion(baseIndexMeta.getSchemaVersion() +
1);
+ rebuildFullSchema();
+ this.schemaChangeVersion++;
+ this.refreshSnapshot = new MTMVRefreshSnapshot();
+ } catch (Exception e) {
+ throw new RuntimeException("alter materialized view add column
failed", e);
+ } finally {
+ writeMvUnlock();
+ }
+ }
+
+ public static String rewriteQuerySqlForAddColumn(String originQuerySql,
String exprSql, String columnName) {
+ SelectColumnClauseIndexFinder finder = new
SelectColumnClauseIndexFinder();
+ ParserRuleContext tree = NereidsParser.toAst(originQuerySql,
DorisParser::singleStatement);
+ finder.visit(tree);
+ Pair<Integer, Integer> selectColumnClauseIndex = finder.getIndex();
+ String escapedColumnName = columnName.replace("`", "``");
+ String newSelectColumnClause =
originQuerySql.substring(selectColumnClauseIndex.first,
+ selectColumnClauseIndex.second + 1)
+ + ", " + exprSql + " AS `" + escapedColumnName + "`";
+ return StringUtils.overlay(originQuerySql, newSelectColumnClause,
+ selectColumnClauseIndex.first, selectColumnClauseIndex.second
+ 1);
+ }
Review Comment:
**Code duplication**: `SelectColumnClauseIndexFinder` (lines 144-178) is a
near-identical copy of `BaseViewInfo.IndexFinder` (BaseViewInfo.java:215-259).
Both extend `DorisParserBaseVisitor<Void>`, both override `visitChildren`,
`visitCte`, and `visitSelectColumnClause` with the same pattern.
The only difference is that `BaseViewInfo.IndexFinder` also captures
`namedExpressionContexts` — but `SelectColumnClauseIndexFinder` could simply
extend or wrap the existing `IndexFinder` instead of duplicating it.
Extract a shared `SelectClauseFinder` utility or have
`SelectColumnClauseIndexFinder` delegate to the existing
`BaseViewInfo.IndexFinder`.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -197,6 +206,80 @@ public MTMVStatus alterStatus(MTMVStatus newStatus) {
}
}
+ public void alterAddColumn(Column newColumn, String exprSql, String
newQuerySql) {
+ writeMvLock();
+ try {
+ querySql = StringUtils.isBlank(newQuerySql)
+ ? rewriteQuerySqlForAddColumn(querySql, exprSql,
newColumn.getName())
+ : newQuerySql;
+
+ MaterializedIndexMeta baseIndexMeta =
getIndexMetaByIndexId(getBaseIndexId());
+ List<Column> newSchema = new
ArrayList<>(baseIndexMeta.getSchema());
+ Column copied = new Column(newColumn);
+ copied.setUniqueId(baseIndexMeta.incAndGetMaxColUniqueId());
+ newSchema.add(copied);
+ baseIndexMeta.setSchema(newSchema);
+ baseIndexMeta.setSchemaVersion(baseIndexMeta.getSchemaVersion() +
1);
+ rebuildFullSchema();
+ this.schemaChangeVersion++;
+ this.refreshSnapshot = new MTMVRefreshSnapshot();
+ } catch (Exception e) {
+ throw new RuntimeException("alter materialized view add column
failed", e);
+ } finally {
+ writeMvUnlock();
+ }
+ }
+
+ public static String rewriteQuerySqlForAddColumn(String originQuerySql,
String exprSql, String columnName) {
+ SelectColumnClauseIndexFinder finder = new
SelectColumnClauseIndexFinder();
+ ParserRuleContext tree = NereidsParser.toAst(originQuerySql,
DorisParser::singleStatement);
Review Comment:
**NPE risk**: `SelectColumnClauseIndexFinder.getIndex()` returns `null` if
the AST contains no `SelectColumnClause` (e.g., `VALUES` statement, or a parse
tree where `visitSelectColumnClause` is never invoked). At line 135,
`selectColumnClauseIndex.first` will throw `NullPointerException`.
This is caught by `alterAddColumn()`'s `catch (Exception)` and wrapped as
`RuntimeException`, but the error message (`"alter materialized view add column
failed"`) gives no indication that a null index was the cause.
Add a null check before line 135:
```java
if (selectColumnClauseIndex == null) {
throw new IllegalStateException("Could not locate SELECT column clause
in query SQL");
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -1313,6 +1314,11 @@ public void processAlterMTMV(AlterMTMV alterMTMV,
boolean isReplay) {
case ALTER_PROPERTY:
mtmv.alterMvProperties(alterMTMV.getMvProperties());
break;
+ case ALTER_ADD_COLUMN:
+ mtmv.alterAddColumn(
Review Comment:
**Exception escape**: `MTMV.alterAddColumn()` wraps all exceptions in
`RuntimeException` (see `catch (Exception e)` at the end of the method).
However, `processAlterMTMV` only catches `UserException` at line 1336, not
`RuntimeException`.
If `alterAddColumn()` fails (e.g., concurrent modification, corrupted index
metadata, or the NPE from Finding 6 above), the `RuntimeException` will
propagate uncaught past the `processAlterMTMV` handler, potentially crashing
the statement execution.
Either:
1. Make `alterAddColumn()` throw a checked exception that `processAlterMTMV`
can handle, or
2. Add a `catch (RuntimeException e)` in `processAlterMTMV` for this case, or
3. Catch `RuntimeException` as well as `UserException` at line 1336.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -197,6 +206,80 @@ public MTMVStatus alterStatus(MTMVStatus newStatus) {
}
}
+ public void alterAddColumn(Column newColumn, String exprSql, String
newQuerySql) {
+ writeMvLock();
+ try {
+ querySql = StringUtils.isBlank(newQuerySql)
+ ? rewriteQuerySqlForAddColumn(querySql, exprSql,
newColumn.getName())
+ : newQuerySql;
+
+ MaterializedIndexMeta baseIndexMeta =
getIndexMetaByIndexId(getBaseIndexId());
+ List<Column> newSchema = new
ArrayList<>(baseIndexMeta.getSchema());
+ Column copied = new Column(newColumn);
+ copied.setUniqueId(baseIndexMeta.incAndGetMaxColUniqueId());
+ newSchema.add(copied);
+ baseIndexMeta.setSchema(newSchema);
+ baseIndexMeta.setSchemaVersion(baseIndexMeta.getSchemaVersion() +
1);
+ rebuildFullSchema();
+ this.schemaChangeVersion++;
+ this.refreshSnapshot = new MTMVRefreshSnapshot();
+ } catch (Exception e) {
+ throw new RuntimeException("alter materialized view add column
failed", e);
+ } finally {
+ writeMvUnlock();
+ }
+ }
Review Comment:
**Duplicated SQL-rewriting pattern**: `rewriteQuerySqlForAddColumn()` (lines
131-141) follows the same AST-then-`StringUtils.overlay` pattern already used
in `BaseViewInfo.rewriteProjectsToUserDefineAlias()`
(BaseViewInfo.java:142-167). Additionally, `BaseViewInfo.rewriteSql()`
(BaseViewInfo.java:129) provides a general-purpose position-keyed SQL rewriting
primitive (`TreeMap<Pair<Integer,Integer>, String>` → rewritten SQL).
There are now **three** copies of this pattern in the codebase (MTMV,
BaseViewInfo, and LogicalPlanBuilderForSyncMv). Consider extracting a shared
`SQLRewriteUtils` utility to avoid triple maintenance of the same logic.
--
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]
