Skip to content

Commit 3eebebb

Browse files
mergify[bot]LiShuMing
mergify[bot]
and
LiShuMing
authored
[BugFix] Fix possible NPE in alter table (backport #62321) (backport #62343) (#62349)
Signed-off-by: shuming.li <ming.moriarty@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: shuming.li <ming.moriarty@gmail.com>
1 parent d9f0f7c commit 3eebebb

File tree

6 files changed

+160
-74
lines changed

6 files changed

+160
-74
lines changed

‎fe/fe-core/src/main/java/com/starrocks/alter/AlterJobExecutor.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,7 @@ public Void visitColumnRenameClause(ColumnRenameClause clause, ConnectContext co
574574
Set<String> modifiedColumns = new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
575575
modifiedColumns.add(clause.getColName());
576576
ErrorReport.wrapWithRuntimeException(() ->
577-
schemaChangeHandler.checkModifiedColumWithMaterializedViews((OlapTable) table, modifiedColumns));
577+
AlterMVJobExecutor.checkModifiedColumWithMaterializedViews((OlapTable) table, modifiedColumns));
578578
GlobalStateMgr.getCurrentState().getLocalMetastore().renameColumn(db, table, clause);
579579

580580
// If modified columns are already done, inactive related mv

‎fe/fe-core/src/main/java/com/starrocks/alter/AlterMVJobExecutor.java

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,15 @@
1616

1717
import com.google.common.collect.Lists;
1818
import com.google.common.collect.Maps;
19+
import com.starrocks.analysis.Expr;
1920
import com.starrocks.analysis.IntLiteral;
21+
import com.starrocks.analysis.SlotRef;
2022
import com.starrocks.analysis.StringLiteral;
2123
import com.starrocks.analysis.TableName;
24+
import com.starrocks.catalog.Column;
2225
import com.starrocks.catalog.Database;
26+
import com.starrocks.catalog.KeysType;
27+
import com.starrocks.catalog.MaterializedIndexMeta;
2328
import com.starrocks.catalog.MaterializedView;
2429
import com.starrocks.catalog.MvId;
2530
import com.starrocks.catalog.MvPlanContext;
@@ -70,6 +75,7 @@
7075
import org.apache.commons.lang3.StringUtils;
7176
import org.threeten.extra.PeriodDuration;
7277

78+
import java.util.ArrayList;
7379
import java.util.List;
7480
import java.util.Map;
7581
import java.util.Set;
@@ -589,4 +595,71 @@ public static void inactiveRelatedMaterializedViews(Database db,
589595
}
590596
}
591597
}
598+
599+
/**
600+
* Check related synchronous materialized views before modified columns, throw exceptions
601+
* if modified columns affect the related rollup/synchronous mvs.
602+
*/
603+
public static void checkModifiedColumWithMaterializedViews(OlapTable olapTable,
604+
Set<String> modifiedColumns) throws DdlException {
605+
if (modifiedColumns == null || modifiedColumns.isEmpty()) {
606+
return;
607+
}
608+
609+
// If there is synchronized materialized view referring the column, throw exception.
610+
if (olapTable.getIndexNameToId().size() > 1) {
611+
Map<Long, MaterializedIndexMeta> metaMap = olapTable.getIndexIdToMeta();
612+
for (Map.Entry<Long, MaterializedIndexMeta> entry : metaMap.entrySet()) {
613+
Long id = entry.getKey();
614+
if (id == olapTable.getBaseIndexId()) {
615+
continue;
616+
}
617+
MaterializedIndexMeta meta = entry.getValue();
618+
List<Column> schema = meta.getSchema();
619+
String indexName = olapTable.getIndexNameById(id);
620+
// ignore agg_keys type because it's like duplicated without agg functions
621+
boolean hasAggregateFunction = olapTable.getKeysType() != KeysType.AGG_KEYS &&
622+
schema.stream().anyMatch(x -> x.isAggregated());
623+
if (hasAggregateFunction) {
624+
for (Column rollupCol : schema) {
625+
String colName = rollupCol.getName();
626+
if (modifiedColumns.contains(colName)) {
627+
throw new DdlException(String.format("Can not drop/modify the column %s, " +
628+
"because the column is used in the related rollup %s, " +
629+
"please drop the rollup index first.", colName, indexName));
630+
}
631+
if (rollupCol.getRefColumns() != null) {
632+
for (SlotRef refColumn : rollupCol.getRefColumns()) {
633+
String refColName = refColumn.getColumnName();
634+
if (modifiedColumns.contains(refColName)) {
635+
String defineExprSql = rollupCol.getDefineExpr() == null ? "" :
636+
rollupCol.getDefineExpr().toSql();
637+
throw new DdlException(String.format("Can not drop/modify the column %s, " +
638+
"because the column is used in the related rollup %s " +
639+
"with the define expr:%s, please drop the rollup index first.",
640+
refColName, indexName, defineExprSql));
641+
}
642+
}
643+
}
644+
}
645+
}
646+
if (meta.getWhereClause() != null) {
647+
Expr whereExpr = meta.getWhereClause();
648+
List<SlotRef> whereSlots = new ArrayList<>();
649+
whereExpr.collect(SlotRef.class, whereSlots);
650+
for (SlotRef refColumn : whereSlots) {
651+
String colName = refColumn.getColumnName();
652+
if (modifiedColumns.contains(colName)) {
653+
String whereExprSql = whereExpr.toSql();
654+
throw new DdlException(String.format("Can not drop/modify the column %s, " +
655+
"because the column is used in the related rollup %s " +
656+
"with the where expr:%s, please drop the rollup index first.",
657+
colName, indexName, whereExprSql));
658+
}
659+
}
660+
}
661+
}
662+
}
663+
}
664+
592665
}

‎fe/fe-core/src/main/java/com/starrocks/alter/SchemaChangeHandler.java

Lines changed: 4 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@
4545
import com.google.common.collect.Sets;
4646
import com.starrocks.analysis.BloomFilterIndexUtil;
4747
import com.starrocks.analysis.ColumnPosition;
48-
import com.starrocks.analysis.Expr;
4948
import com.starrocks.analysis.SlotRef;
5049
import com.starrocks.binlog.BinlogConfig;
5150
import com.starrocks.catalog.AggregateType;
@@ -1877,7 +1876,7 @@ public int getAsInt() {
18771876
DropColumnClause dropColumnClause = (DropColumnClause) alterClause;
18781877
// check relative mvs with the modified column
18791878
Set<String> modifiedColumns = Set.of(dropColumnClause.getColName());
1880-
checkModifiedColumWithMaterializedViews(olapTable, modifiedColumns);
1879+
AlterMVJobExecutor.checkModifiedColumWithMaterializedViews(olapTable, modifiedColumns);
18811880

18821881
// drop column and drop indexes on this column
18831882
fastSchemaEvolution &=
@@ -1888,7 +1887,7 @@ public int getAsInt() {
18881887

18891888
// check relative mvs with the modified column
18901889
Set<String> modifiedColumns = Set.of(modifyColumnClause.getColumn().getName());
1891-
checkModifiedColumWithMaterializedViews(olapTable, modifiedColumns);
1890+
AlterMVJobExecutor.checkModifiedColumWithMaterializedViews(olapTable, modifiedColumns);
18921891

18931892
// modify column
18941893
fastSchemaEvolution &= processModifyColumn(modifyColumnClause, olapTable, indexSchemaMap);
@@ -1903,7 +1902,7 @@ public int getAsInt() {
19031902
}
19041903
AddFieldClause addFieldClause = (AddFieldClause) alterClause;
19051904
modifyFieldColumns = Set.of(addFieldClause.getColName());
1906-
checkModifiedColumWithMaterializedViews(olapTable, modifyFieldColumns);
1905+
AlterMVJobExecutor.checkModifiedColumWithMaterializedViews(olapTable, modifyFieldColumns);
19071906
int id = colUniqueIdSupplier.getAsInt();
19081907
processAddField((AddFieldClause) alterClause, olapTable, indexSchemaMap, id, newIndexes);
19091908
} else if (alterClause instanceof DropFieldClause) {
@@ -1917,7 +1916,7 @@ public int getAsInt() {
19171916
}
19181917
DropFieldClause dropFieldClause = (DropFieldClause) alterClause;
19191918
modifyFieldColumns = Set.of(dropFieldClause.getColName());
1920-
checkModifiedColumWithMaterializedViews(olapTable, modifyFieldColumns);
1919+
AlterMVJobExecutor.checkModifiedColumWithMaterializedViews(olapTable, modifyFieldColumns);
19211920
processDropField((DropFieldClause) alterClause, olapTable, indexSchemaMap, newIndexes);
19221921
} else if (alterClause instanceof ReorderColumnsClause) {
19231922
// reorder column
@@ -1975,68 +1974,6 @@ public int getAsInt() {
19751974
}
19761975
}
19771976

1978-
/**
1979-
* Check related synchronous materialized views before modified columns, throw exceptions
1980-
* if modified columns affect the related rollup/synchronous mvs.
1981-
*/
1982-
public void checkModifiedColumWithMaterializedViews(OlapTable olapTable,
1983-
Set<String> modifiedColumns) throws DdlException {
1984-
if (modifiedColumns == null || modifiedColumns.isEmpty()) {
1985-
return;
1986-
}
1987-
1988-
// If there is synchronized materialized view referring the column, throw exception.
1989-
if (olapTable.getIndexNameToId().size() > 1) {
1990-
Map<Long, MaterializedIndexMeta> metaMap = olapTable.getIndexIdToMeta();
1991-
for (Map.Entry<Long, MaterializedIndexMeta> entry : metaMap.entrySet()) {
1992-
Long id = entry.getKey();
1993-
if (id == olapTable.getBaseIndexId()) {
1994-
continue;
1995-
}
1996-
MaterializedIndexMeta meta = entry.getValue();
1997-
List<Column> schema = meta.getSchema();
1998-
// ignore agg_keys type because it's like duplicated without agg functions
1999-
boolean hasAggregateFunction = olapTable.getKeysType() != KeysType.AGG_KEYS &&
2000-
schema.stream().anyMatch(x -> x.isAggregated());
2001-
if (hasAggregateFunction) {
2002-
for (Column rollupCol : schema) {
2003-
if (modifiedColumns.contains(rollupCol.getName())) {
2004-
throw new DdlException(String.format("Can not drop/modify the column %s, " +
2005-
"because the column is used in the related rollup %s, " +
2006-
"please drop the rollup index first.",
2007-
rollupCol.getName(), olapTable.getIndexNameById(meta.getIndexId())));
2008-
}
2009-
if (rollupCol.getRefColumns() != null) {
2010-
for (SlotRef refColumn : rollupCol.getRefColumns()) {
2011-
if (modifiedColumns.contains(refColumn.getColumnName())) {
2012-
throw new DdlException(String.format("Can not drop/modify the column %s, " +
2013-
"because the column is used in the related rollup %s " +
2014-
"with the define expr:%s, please drop the rollup index first.",
2015-
rollupCol.getName(), olapTable.getIndexNameById(meta.getIndexId()),
2016-
rollupCol.getDefineExpr().toSql()));
2017-
}
2018-
}
2019-
}
2020-
}
2021-
}
2022-
if (meta.getWhereClause() != null) {
2023-
Expr whereExpr = meta.getWhereClause();
2024-
List<SlotRef> whereSlots = new ArrayList<>();
2025-
whereExpr.collect(SlotRef.class, whereSlots);
2026-
for (SlotRef refColumn : whereSlots) {
2027-
if (modifiedColumns.contains(refColumn.getColumnName())) {
2028-
throw new DdlException(String.format("Can not drop/modify the column %s, " +
2029-
"because the column is used in the related rollup %s " +
2030-
"with the where expr:%s, please drop the rollup index first.",
2031-
refColumn.getColumn().getName(), olapTable.getIndexNameById(meta.getIndexId()),
2032-
meta.getWhereClause().toSql()));
2033-
}
2034-
}
2035-
}
2036-
}
2037-
}
2038-
}
2039-
20401977
@Override
20411978
public ShowResultSet process(List<AlterClause> alterClauses, Database db, OlapTable olapTable)
20421979
throws UserException {

‎fe/fe-core/src/test/java/com/starrocks/alter/SchemaChangeHandlerWithMVTest.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.apache.logging.log4j.Logger;
2929
import org.junit.Assert;
3030
import org.junit.FixMethodOrder;
31+
import org.junit.jupiter.api.Assertions;
3132
import org.junit.jupiter.api.Test;
3233
import org.junit.runners.MethodSorters;
3334

@@ -216,9 +217,9 @@ public void testModifyColumnsWithAggregateRollup3() {
216217
"group by timestamp",
217218
"alter table sc_dup3 drop column error_code");
218219
} catch (Exception e) {
219-
Assert.assertTrue(e.getMessage().contains("Can not drop/modify the column mv_count_error_code, because the column " +
220-
"is used in the related rollup mv1 with the define expr:" +
221-
"CASE WHEN `test`.`sc_dup3`.`error_code` IS NULL THEN 0 ELSE 1 END, please drop the rollup index first."));
220+
Assertions.assertTrue(
221+
e.getMessage().contains("Can not drop/modify the column error_code, " +
222+
"because the column is used in the related rollup mv1 with the define exp"), e.getMessage());
222223
}
223224
}
224225

@@ -231,9 +232,8 @@ public void testModifyColumnsWithAggregateRollup4() {
231232
"group by timestamp",
232233
"alter table sc_dup3 modify column error_code BIGINT");
233234
} catch (Exception e) {
234-
Assert.assertTrue(e.getMessage().contains("Can not drop/modify the column mv_count_error_code, because " +
235-
"the column is used in the related rollup mv1 with the define expr:" +
236-
"CASE WHEN `test`.`sc_dup3`.`error_code` IS NULL THEN 0 ELSE 1 END, please drop the rollup index first."));
235+
Assertions.assertTrue(e.getMessage().contains("Can not drop/modify the column error_code, because " +
236+
"the column is used in the related rollup mv1 with the define expr"), e.getMessage());
237237
}
238238
}
239239

‎test/sql/test_alter_table/R/test_alter_table_with_mv

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
-- name: test_alter_table_with_mv
2+
CREATE TABLE t0(k0 BIGINT, k1 DATETIME, v0 INT, v1 VARCHAR(100))
3+
distributed by hash(k0)
4+
order by (v0)
5+
properties('replication_num'='1');
6+
-- result:
7+
-- !result
8+
INSERT INTO t0 VALUES(0, '2024-01-01 00:00:00', 10, '100');
9+
-- result:
10+
-- !result
11+
CREATE MATERIALIZED VIEW test_mv1 AS SELECT k0, v1 FROM t0 WHERE v0 > 10;
12+
-- result:
13+
-- !result
14+
function: wait_table_rollup_finish()
15+
-- result:
16+
None
17+
-- !result
18+
CREATE MATERIALIZED VIEW test_mv2 AS SELECT k0, v1, k1, v0 FROM t0 WHERE v0 > 10 and k1 = '2024-01-01 00:00:00';
19+
-- result:
20+
-- !result
21+
function: wait_table_rollup_finish()
22+
-- result:
23+
None
24+
-- !result
25+
ALTER TABLE t0 DROP COLUMN k1;
26+
-- result:
27+
[REGEX].*Can not drop/modify the column k1, because the column is used in the related rollup.*
28+
-- !result
29+
ALTER TABLE t0 DROP COLUMN v0;
30+
-- result:
31+
[REGEX].*Can not drop/modify the column v0, because the column is used in the related rollup.*
32+
-- !result
33+
ALTER TABLE t0 DROP COLUMN v1, DROP COLUMN v0;
34+
-- result:
35+
[REGEX].*Can not drop/modify the column v0, because the column is used in the related rollup.*
36+
-- !result
37+
ALTER TABLE t0 DROP COLUMN v1, DROP COLUMN v100;
38+
-- result:
39+
E: (1064, 'Column does not exists: v100')
40+
-- !result
41+
DESC t0;
42+
-- result:
43+
k0 bigint YES true None
44+
k1 datetime YES true None
45+
v0 int YES true None
46+
v1 varchar(100) YES false None
47+
-- !result
48+
SELECT * from t0 order by k0;
49+
-- result:
50+
0 2024-01-01 00:00:00 10 100
51+
-- !result

‎test/sql/test_alter_table/T/test_alter_table_with_mv

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
-- name: test_alter_table_with_mv
2+
3+
CREATE TABLE t0(k0 BIGINT, k1 DATETIME, v0 INT, v1 VARCHAR(100))
4+
distributed by hash(k0)
5+
order by (v0)
6+
properties('replication_num'='1');
7+
8+
INSERT INTO t0 VALUES(0, '2024-01-01 00:00:00', 10, '100');
9+
10+
CREATE MATERIALIZED VIEW test_mv1 AS SELECT k0, v1 FROM t0 WHERE v0 > 10;
11+
function: wait_table_rollup_finish()
12+
CREATE MATERIALIZED VIEW test_mv2 AS SELECT k0, v1, k1, v0 FROM t0 WHERE v0 > 10 and k1 = '2024-01-01 00:00:00';
13+
function: wait_table_rollup_finish()
14+
15+
ALTER TABLE t0 DROP COLUMN k1;
16+
17+
ALTER TABLE t0 DROP COLUMN v0;
18+
19+
ALTER TABLE t0 DROP COLUMN v1, DROP COLUMN v0;
20+
21+
ALTER TABLE t0 DROP COLUMN v1, DROP COLUMN v100;
22+
23+
DESC t0;
24+
25+
SELECT * from t0 order by k0;

0 commit comments

Comments
 (0)