[Bug](materialized-view) add limit for drop column on mv (apache#24493)

add limit for drop column on mv
xinyiZzz · Sep 19, 2023 · 5e4ab7c · 5e4ab7c
1 parent ee56783
commit 5e4ab7c
Show file tree

Hide file tree

Showing 6 changed files with 52 additions and 14 deletions.
diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java
@@ -33,6 +33,7 @@
 import org.apache.doris.analysis.ModifyTablePropertiesClause;
 import org.apache.doris.analysis.ReorderColumnsClause;
 import org.apache.doris.analysis.ShowAlterStmt.AlterType;
+import org.apache.doris.analysis.SlotRef;
 import org.apache.doris.catalog.AggregateType;
 import org.apache.doris.catalog.BinlogConfig;
 import org.apache.doris.catalog.Column;
@@ -404,21 +405,28 @@ private boolean processDropColumn(DropColumnClause alterClause, OlapTable olapTa
                 throw new DdlException("Column does not exists: " + dropColName);
             }
 
-            // remove column in rollup index if exists (i = 1 to skip base index)
             for (int i = 1; i < indexIds.size(); i++) {
                 List<Column> rollupSchema = indexSchemaMap.get(indexIds.get(i));
                 Iterator<Column> iter = rollupSchema.iterator();
                 while (iter.hasNext()) {
                     Column column = iter.next();
-                    if (column.getName().equalsIgnoreCase(dropColName)) {
-                        if (column.isKey()) {
-                            lightSchemaChange = false;
+                    boolean containedByMV = column.getName().equalsIgnoreCase(dropColName);
+                    if (!containedByMV && column.getDefineExpr() != null) {
+                        List<SlotRef> slots = new ArrayList<>();
+                        column.getDefineExpr().collect(SlotRef.class, slots);
+                        for (SlotRef slot : slots) {
+                            if (slot.getColumnName().equalsIgnoreCase(dropColName)) {
+                                containedByMV = true;
+                                break;
+                            }
                         }
-                        iter.remove();
-                        break;
+                    }
+                    if (containedByMV) {
+                        throw new DdlException("Can not drop column contained by mv, mv="
+                                + olapTable.getIndexNameById(indexIds.get(i)));
                     }
                 }
-            } // end for index names
+            }
         } else {
             // if specify rollup index, only drop column from specified rollup index
             // find column

diff --git a/fe/fe-core/src/test/java/org/apache/doris/alter/SchemaChangeHandlerTest.java b/fe/fe-core/src/test/java/org/apache/doris/alter/SchemaChangeHandlerTest.java
@@ -197,16 +197,20 @@ public void testAggAddOrDropColumn() throws Exception {
         //process agg drop value column with rollup schema change
         String dropRollUpValColStmtStr = "alter table test.sc_agg drop column max_dwell_time";
         AlterTableStmt dropRollUpValColStmt = (AlterTableStmt) parseAndAnalyzeStmt(dropRollUpValColStmtStr);
-        Env.getCurrentEnv().getAlterInstance().processAlterTable(dropRollUpValColStmt);
-        jobSize++;
+        try {
+            Env.getCurrentEnv().getAlterInstance().processAlterTable(dropRollUpValColStmt);
+            Assertions.assertTrue(false);
+        } catch (Exception e) {
+            LOG.info("{}", e);
+        }
         //check alter job, need create job
         LOG.info("alterJobs:{}", alterJobs);
         Assertions.assertEquals(jobSize, alterJobs.size());
         waitAlterJobDone(materializedViewAlterJobs);
 
         tbl.readLock();
         try {
-            Assertions.assertEquals(9, tbl.getBaseSchema().size());
+            Assertions.assertEquals(10, tbl.getBaseSchema().size());
             String baseIndexName = tbl.getIndexNameById(tbl.getBaseIndexId());
             Assertions.assertEquals(baseIndexName, tbl.getName());
             MaterializedIndexMeta indexMeta = tbl.getIndexMetaByIndexId(tbl.getBaseIndexId());
@@ -226,7 +230,7 @@ public void testAggAddOrDropColumn() throws Exception {
 
         tbl.readLock();
         try {
-            Assertions.assertEquals(11, tbl.getBaseSchema().size());
+            Assertions.assertEquals(12, tbl.getBaseSchema().size());
             String baseIndexName = tbl.getIndexNameById(tbl.getBaseIndexId());
             Assertions.assertEquals(baseIndexName, tbl.getName());
             MaterializedIndexMeta indexMeta = tbl.getIndexMetaByIndexId(tbl.getBaseIndexId());

diff --git a/regression-test/suites/schema_change_p0/test_agg_mv_schema_change.groovy b/regression-test/suites/schema_change_p0/test_agg_mv_schema_change.groovy
@@ -118,7 +118,14 @@ suite ("test_agg_mv_schema_change") {
 
         qt_sc """ select * from ${tableName} order by user_id"""
 
-        // drop value column with mv, not light schema change
+        test {
+            sql "ALTER TABLE ${tableName} DROP COLUMN cost"
+            exception "Can not drop column contained by mv, mv=mv1"
+        }
+
+        sql""" drop materialized view mv1 on ${tableName}; """
+
+        // drop column
         sql """
             ALTER TABLE ${tableName} DROP COLUMN cost
             """

diff --git a/regression-test/suites/schema_change_p0/test_agg_rollup_schema_change.groovy b/regression-test/suites/schema_change_p0/test_agg_rollup_schema_change.groovy
@@ -114,7 +114,14 @@ suite ("test_agg_rollup_schema_change") {
 
         qt_sc """ select * from ${tableName} order by user_id """
 
-        // drop value column with rollup, not light schema change
+        test {
+            sql "ALTER TABLE ${tableName} DROP COLUMN cost"
+            exception "Can not drop column contained by mv, mv=rollup_cost"
+        }
+
+        sql""" drop materialized view rollup_cost on ${tableName}; """
+
+        // drop column
         sql """
             ALTER TABLE ${tableName} DROP COLUMN cost
             """

diff --git a/regression-test/suites/schema_change_p0/test_uniq_mv_schema_change.groovy b/regression-test/suites/schema_change_p0/test_uniq_mv_schema_change.groovy
@@ -140,12 +140,17 @@ suite ("test_uniq_mv_schema_change") {
 
     qt_sc """ select count(*) from ${tableName} """
 
+    test {
+        sql "ALTER TABLE ${tableName} DROP COLUMN cost"
+        exception "Can not drop column contained by mv, mv=mv2"
+    }
+
+    sql""" drop materialized view mv2 on ${tableName}; """
 
     // drop column
     sql """
           ALTER TABLE ${tableName} DROP COLUMN cost
           """
-
     qt_sc """ select * from ${tableName} where user_id = 3 """
 
 

diff --git a/regression-test/suites/schema_change_p0/test_uniq_rollup_schema_change.groovy b/regression-test/suites/schema_change_p0/test_uniq_rollup_schema_change.groovy
@@ -134,6 +134,13 @@ suite ("test_uniq_rollup_schema_change") {
 
     qt_sc """ select count(*) from ${tableName} """
 
+    test {
+        sql "ALTER TABLE ${tableName} DROP COLUMN cost"
+        exception "Can not drop column contained by mv, mv=rollup_cost"
+    }
+
+    sql""" drop materialized view rollup_cost on ${tableName}; """
+
     // drop column
     sql """
           ALTER TABLE ${tableName} DROP COLUMN cost