Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import org.apache.doris.datasource.iceberg.IcebergExternalTable;
import org.apache.doris.info.TableNameInfoUtils;
import org.apache.doris.mtmv.BaseTableInfo;
import org.apache.doris.mtmv.MTMVAlterOpType;
import org.apache.doris.nereids.trees.plans.commands.AlterSystemCommand;
import org.apache.doris.nereids.trees.plans.commands.AlterTableCommand;
import org.apache.doris.nereids.trees.plans.commands.AlterViewCommand;
Expand Down Expand Up @@ -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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

alterMTMV.getNewColumn(),
alterMTMV.getExprSql(), alterMTMV.getNewQuerySql());
break;
case ADD_TASK:
alterSuccess = mtmv.addTaskResult(alterMTMV.getTask(), alterMTMV.getRelation(),
alterMTMV.getPartitionSnapshots(),
Expand All @@ -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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
// 4. log it and replay it in the follower
if (!isReplay && alterSuccess) {
Env.getCurrentEnv().getEditLog().logAlterMTMV(alterMTMV);
Expand Down
9 changes: 9 additions & 0 deletions fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@
import org.apache.doris.nereids.trees.plans.commands.UninstallPluginCommand;
import org.apache.doris.nereids.trees.plans.commands.info.AddPartitionLikeOp;
import org.apache.doris.nereids.trees.plans.commands.info.AddPartitionOp;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMTMVAddColumnInfo;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMTMVPropertyInfo;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMTMVRefreshInfo;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMultiPartitionOp;
Expand Down Expand Up @@ -7569,6 +7570,14 @@ public StatisticsJobAppender getStatisticsJobAppender() {
return statisticsJobAppender;
}

public void alterMTMVAddColumnInfo(AlterMTMVAddColumnInfo info) {
AlterMTMV alter = new AlterMTMV(info.getMvName(),
info.getColumn().translateToCatalogStyleForSchemaChange(),
info.getExprSql(),
info.getRewrittenQuerySql(), MTMVAlterOpType.ALTER_ADD_COLUMN);
this.alter.processAlterMTMV(alter, false);
}

public void alterMTMVRefreshInfo(AlterMTMVRefreshInfo info) {
AlterMTMV alter = new AlterMTMV(info.getMvName(), info.getRefreshInfo(), MTMVAlterOpType.ALTER_REFRESH_INFO);
this.alter.processAlterMTMV(alter, false);
Expand Down
85 changes: 84 additions & 1 deletion fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.doris.catalog.info.TableNameInfo;
import org.apache.doris.common.AnalysisException;
import org.apache.doris.common.Config;
import org.apache.doris.common.Pair;
import org.apache.doris.common.util.PropertyAnalyzer;
import org.apache.doris.datasource.CatalogMgr;
import org.apache.doris.job.common.TaskStatus;
Expand All @@ -44,18 +45,26 @@
import org.apache.doris.mtmv.MTMVRelation;
import org.apache.doris.mtmv.MTMVSnapshotIf;
import org.apache.doris.mtmv.MTMVStatus;
import org.apache.doris.nereids.DorisParser;
import org.apache.doris.nereids.DorisParserBaseVisitor;
import org.apache.doris.nereids.parser.NereidsParser;
import org.apache.doris.nereids.rules.analysis.SessionVarGuardRewriter;
import org.apache.doris.qe.ConnectContext;

import com.google.common.collect.Maps;
import com.google.common.collect.Sets;
import com.google.gson.annotations.SerializedName;
import org.apache.commons.collections4.MapUtils;
import org.antlr.v4.runtime.ParserRuleContext;
import org.antlr.v4.runtime.tree.ParseTree;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

import org.antlr.v4.runtime.tree.RuleNode;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

import org.apache.commons.collections.MapUtils;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accidental library downgrade: The import changed from org.apache.commons.collections4.MapUtils (commons-collections4 v4.x, the version explicitly declared in fe/pom.xml) to org.apache.commons.collections.MapUtils (commons-collections v3.x, not declared as a direct dependency).

  • v4 provides generic type-safe MapUtils.isEmpty(Map<K,V>); v3 takes raw Map.
  • The entire rest of the codebase uses org.apache.commons.collections4.
  • This appears to be an accidental IDE auto-import — the MapUtils.isEmpty() calls at lines 781/787 are unchanged and unrelated to the ADD COLUMN feature.

Fix: Revert this import to org.apache.commons.collections4.MapUtils.

import org.apache.commons.lang3.StringUtils;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

import org.apache.logging.log4j.LogManager;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test

import org.apache.logging.log4j.Logger;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
Expand Down Expand Up @@ -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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

rewriteCacheGeneration++;
cacheWithGuard = null;
cacheWithoutGuard = null;

Add these three lines here alongside schemaChangeVersion++.

this.schemaChangeVersion++;
this.refreshSnapshot = new MTMVRefreshSnapshot();
} catch (Exception e) {
throw new RuntimeException("alter materialized view add column failed", e);
} finally {
writeMvUnlock();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public static String rewriteQuerySqlForAddColumn(String originQuerySql, String exprSql, String columnName) {
SelectColumnClauseIndexFinder finder = new SelectColumnClauseIndexFinder();
ParserRuleContext tree = NereidsParser.toAst(originQuerySql, DorisParser::singleStatement);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

if (selectColumnClauseIndex == null) {
    throw new IllegalStateException("Could not locate SELECT column clause in query SQL");
}

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);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private static class SelectColumnClauseIndexFinder extends DorisParserBaseVisitor<Void> {
private boolean found = false;
private Pair<Integer, Integer> index;

@Override
public Void visitChildren(RuleNode node) {
if (found) {
return null;
}
int n = node.getChildCount();
for (int i = 0; i < n; ++i) {
ParseTree child = node.getChild(i);
child.accept(this);
}
return null;
}

@Override
public Void visitCte(DorisParser.CteContext ctx) {
return null;
}

@Override
public Void visitSelectColumnClause(DorisParser.SelectColumnClauseContext ctx) {
if (found) {
return null;
}
found = true;
index = Pair.of(ctx.getStart().getStartIndex(), ctx.getStop().getStopIndex());
return null;
}

public Pair<Integer, Integer> getIndex() {
return index;
}
}

public void processBaseViewChange(String schemaChangeDetail) {
writeMvLock();
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ public enum MTMVAlterOpType {
ALTER_REFRESH_INFO,
ALTER_STATUS,
ALTER_PROPERTY,
ALTER_ADD_COLUMN,
ADD_TASK;
}
Original file line number Diff line number Diff line change
Expand Up @@ -925,6 +925,7 @@
import org.apache.doris.nereids.trees.plans.commands.info.AddPartitionOp;
import org.apache.doris.nereids.trees.plans.commands.info.AddRollupOp;
import org.apache.doris.nereids.trees.plans.commands.info.AlterLoadErrorUrlOp;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMTMVAddColumnInfo;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMTMVInfo;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMTMVPropertyInfo;
import org.apache.doris.nereids.trees.plans.commands.info.AlterMTMVRefreshInfo;
Expand Down Expand Up @@ -1939,6 +1940,10 @@ public AlterMTMVCommand visitAlterMTMV(AlterMTMVContext ctx) {
Map<String, String> properties = ctx.propertyClause() != null
? Maps.newHashMap(visitPropertyClause(ctx.propertyClause())) : Maps.newHashMap();
alterMTMVInfo = new AlterMTMVReplaceInfo(mvName, newName, properties);
} else if (ctx.colName != null) {
String columnName = ctx.colName.getText();
String exprSql = getOriginSql(ctx.expression());
alterMTMVInfo = new AlterMTMVAddColumnInfo(mvName, columnName, exprSql);
}
return new AlterMTMVCommand(alterMTMVInfo);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

package org.apache.doris.nereids.trees.plans.commands.info;

import org.apache.doris.catalog.Env;
import org.apache.doris.catalog.MTMV;
import org.apache.doris.catalog.TableIf.TableType;
import org.apache.doris.common.DdlException;
import org.apache.doris.common.UserException;
import org.apache.doris.mtmv.MTMVPlanUtil;
import org.apache.doris.nereids.exceptions.AnalysisException;
import org.apache.doris.qe.ConnectContext;

import org.apache.commons.collections.CollectionUtils;
import org.apache.commons.lang3.StringUtils;

import java.util.List;
import java.util.Objects;

/**
* alter mtmv add column info.
* Grammar: ALTER MATERIALIZED VIEW mv ADD COLUMN colName AS expression.
* The new column type is fully inferred from the expression, users do NOT specify a type.
*/
public class AlterMTMVAddColumnInfo extends AlterMTMVInfo {
private final String columnName;
private final String exprSql;
private ColumnDefinition analyzedColumn;
private String rewrittenQuerySql;

public AlterMTMVAddColumnInfo(TableNameInfo mvName, String columnName, String exprSql) {
super(mvName);
this.columnName = Objects.requireNonNull(columnName, "require columnName");
this.exprSql = Objects.requireNonNull(exprSql, "require exprSql object");
}

@Override
public void analyze(ConnectContext ctx) throws AnalysisException {
super.analyze(ctx);
if (StringUtils.isBlank(columnName)) {
throw new AnalysisException("ADD COLUMN must specify a column name");
}
if (StringUtils.isBlank(exprSql)) {
throw new AnalysisException("ADD COLUMN must specify expression by `AS expression`");
}
try {
MTMV mtmv = (MTMV) Env.getCurrentInternalCatalog().getDbOrAnalysisException(mvName.getDb())
.getTableOrDdlException(mvName.getTbl(), TableType.MATERIALIZED_VIEW);
if (mtmv.getColumn(columnName) != null) {
throw new AnalysisException("Column already exists: " + columnName);
}
inferColumnFromExpression(mtmv, ctx);
} catch (DdlException | org.apache.doris.common.AnalysisException e) {
throw new AnalysisException(e.getMessage(), e);
}
}

private void inferColumnFromExpression(MTMV mtmv, ConnectContext ctx) {
rewrittenQuerySql = MTMV.rewriteQuerySqlForAddColumn(mtmv.getQuerySql(), exprSql, columnName);
List<ColumnDefinition> allColumns = MTMVPlanUtil.generateColumnsBySql(
rewrittenQuerySql,
ctx,
mtmv.getMvPartitionInfo() == null ? null : mtmv.getMvPartitionInfo().getPartitionCol(),
mtmv.getDistributionColumnNames(),
null,
mtmv.getTableProperty() == null ? null : mtmv.getTableProperty().getProperties());
if (CollectionUtils.isEmpty(allColumns) || allColumns.size() != mtmv.getBaseSchema().size() + 1) {
throw new AnalysisException("failed to infer added column by expression");
}
analyzedColumn = allColumns.get(allColumns.size() - 1);
}

@Override
public void run() throws UserException {
Env.getCurrentEnv().alterMTMVAddColumnInfo(this);
}

public ColumnDefinition getColumn() {
return analyzedColumn;
}

public String getExprSql() {
return exprSql;
}

public String getRewrittenQuerySql() {
return rewrittenQuerySql;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.doris.persist;

import org.apache.doris.catalog.info.TableNameInfo;
import org.apache.doris.catalog.Column;
import org.apache.doris.common.io.Text;
import org.apache.doris.common.io.Writable;
import org.apache.doris.job.extensions.mtmv.MTMVTask;
Expand All @@ -37,6 +38,12 @@
import java.util.Objects;

public class AlterMTMV implements Writable {
@SerializedName("nc")
private Column newColumn;
@SerializedName("eq")
private String exprSql;
@SerializedName("nqs")
private String newQuerySql;
@SerializedName("ot")
private MTMVAlterOpType opType;
@SerializedName("mn")
Expand All @@ -63,6 +70,20 @@ public AlterMTMV(TableNameInfo mvName, MTMVRefreshInfo refreshInfo, MTMVAlterOpT
this.needRebuildJob = true;
}

public AlterMTMV(TableNameInfo mvName, Column newColumn, String exprSql, MTMVAlterOpType opType) {
this(mvName, newColumn, exprSql, null, opType);
}

public AlterMTMV(TableNameInfo mvName, Column newColumn, String exprSql,
String newQuerySql, MTMVAlterOpType opType) {
this.mvName = Objects.requireNonNull(mvName, "require mvName object");
this.newColumn = Objects.requireNonNull(newColumn, "require addcolumn object");
this.exprSql = Objects.requireNonNull(exprSql, "require exprSql object");
this.newQuerySql = newQuerySql;
this.opType = Objects.requireNonNull(opType, "require opType object");
this.needRebuildJob = true;
}

public AlterMTMV(TableNameInfo mvName, MTMVAlterOpType opType) {
this.mvName = Objects.requireNonNull(mvName, "require mvName object");
this.opType = Objects.requireNonNull(opType, "require opType object");
Expand Down Expand Up @@ -112,6 +133,18 @@ public MTMVAlterOpType getOpType() {
return opType;
}

public Column getNewColumn() {
return newColumn;
}

public String getExprSql() {
return exprSql;
}

public String getNewQuerySql() {
return newQuerySql;
}

public MTMVTask getTask() {
return task;
}
Expand Down
Loading
Loading