From d4562f167565c6cc90a46276ccb7343579787a96 Mon Sep 17 00:00:00 2001 From: Yukang-Lian Date: Fri, 28 Aug 2026 02:50:58 +0800 Subject: [PATCH] [fix](fe) Avoid reusing dropped column unique ids ### What problem does this PR solve? Issue Number: None Related PR: #63173 Problem Summary: Light schema change uses column unique IDs to match FE slots with columns stored in existing segments. Some supported legacy metadata can contain a maxColUniqueId lower than an ID that is still present in the current schema. The DROP COLUMN path replaced the index schema before preserving its unique-ID high-water mark. Dropping the highest-ID column could therefore lose that historical ID, and a later ADD COLUMN could assign it again. Existing segments could then be interpreted as containing values for the new column. This change normalizes maxColUniqueId when metadata is deserialized and preserves the maximum from both the old and new schemas during light schema changes. It prevents future drops from losing a high-water mark that is still available in current metadata. It cannot reconstruct an ID that was already absent from both the schema and maxColUniqueId before upgrade. ### Release note Prevent future light schema changes from reusing dropped column unique IDs when a legacy high-water mark is stale. Metadata that had already lost its historical high-water mark before upgrade is not repaired by this change. ### Check List (For Author) - Test: Regression test / Unit Test - `./run-fe-ut.sh --run 'org.apache.doris.alter.SchemaChangeHandlerTest#testReaddDroppedValueColumnUsesNewUniqueId,org.apache.doris.catalog.MaterializedIndexMetaTest'` - `LOCAL_DORIS_PATH=$PWD/.doris-compose ./run-regression-test.sh --run -d fault_injection_p0 -s test_readd_dropped_column_unique_id -image doris-pr63173-test:latest -runMode not_cloud` - `NODE_OPTIONS=--openssl-legacy-provider ./build.sh --fe` - Behavior changed: Yes. Light schema changes preserve the column unique-ID high-water mark so newly added columns do not reuse IDs still known to the metadata. - Does this need documentation: No Co-authored-by: Siyang Tang --- .../doris/alter/SchemaChangeHandler.java | 18 +++-- .../doris/catalog/MaterializedIndexMeta.java | 15 ++++- .../doris/alter/SchemaChangeHandlerTest.java | 37 ++++++++++ .../catalog/MaterializedIndexMetaTest.java | 64 ++++++++++++++++++ .../test_readd_dropped_column_unique_id.out | 8 +++ ...test_readd_dropped_column_unique_id.groovy | 67 +++++++++++++++++++ 6 files changed, 204 insertions(+), 5 deletions(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/catalog/MaterializedIndexMetaTest.java create mode 100644 regression-test/data/fault_injection_p0/test_readd_dropped_column_unique_id.out create mode 100644 regression-test/suites/fault_injection_p0/test_readd_dropped_column_unique_id.groovy 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 index 8396e36bdb7f16..64e18ad273266d 100644 --- 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 @@ -70,6 +70,7 @@ import org.apache.doris.common.UserException; import org.apache.doris.common.util.BufferSizeUtil; import org.apache.doris.common.util.DbUtil; +import org.apache.doris.common.util.DebugPointUtil; import org.apache.doris.common.util.DynamicPartitionUtil; import org.apache.doris.common.util.ListComparator; import org.apache.doris.common.util.PropertyAnalyzer; @@ -3731,6 +3732,18 @@ public void updateBaseIndexSchema(OlapTable olapTable, Map indexSchema = indexSchemaMap.get(indexIds.get(i)); MaterializedIndexMeta currentIndexMeta = olapTable.getIndexMetaByIndexId(indexIds.get(i)); + String forceStaleMaxColUniqueIdTable = DebugPointUtil.getDebugParamOrDefault( + "FE.SchemaChangeHandler.updateBaseIndexSchema.forceStaleMaxColUniqueId", "table_name", ""); + if (olapTable.getName().equals(forceStaleMaxColUniqueIdTable)) { + currentIndexMeta.setMaxColUniqueId(Column.COLUMN_UNIQUE_ID_INIT_VALUE); + } + + // Preserve unique ids from the old schema before replacing it. Otherwise a stale maxColUniqueId can + // forget the highest id when its column is dropped, allowing a later column to reuse that id. + int maxColUniqueId = currentIndexMeta.getMaxColUniqueId(); + for (Column column : currentIndexMeta.getSchema()) { + maxColUniqueId = Math.max(maxColUniqueId, column.getUniqueId()); + } currentIndexMeta.setSchema(indexSchema); int currentSchemaVersion = currentIndexMeta.getSchemaVersion(); @@ -3738,11 +3751,8 @@ public void updateBaseIndexSchema(OlapTable olapTable, Map maxColUniqueId) { - maxColUniqueId = column.getUniqueId(); - } + maxColUniqueId = Math.max(maxColUniqueId, column.getUniqueId()); } currentIndexMeta.setMaxColUniqueId(maxColUniqueId); currentIndexMeta.setIndexes(indexes); diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java index dca0f1d8234b56..297aea722d93a3 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/MaterializedIndexMeta.java @@ -279,6 +279,7 @@ public void gsonPostProcess() throws IOException { if (sessionVariables == null) { sessionVariables = Maps.newHashMap(); } + refreshMaxColUniqueId(); initColumnNameMap(); } @@ -349,7 +350,7 @@ public int incAndGetMaxColUniqueId() { } public int getMaxColUniqueId() { - return this.maxColUniqueId; + return Math.max(maxColUniqueId, getSchemaMaxColUniqueId()); } public void setMaxColUniqueId(int maxColUniqueId) { @@ -367,6 +368,18 @@ public void initSchemaColumnUniqueId() { }); } + private void refreshMaxColUniqueId() { + maxColUniqueId = getMaxColUniqueId(); + } + + private int getSchemaMaxColUniqueId() { + int schemaMaxColUniqueId = Column.COLUMN_UNIQUE_ID_INIT_VALUE; + for (Column column : schema) { + schemaMaxColUniqueId = Math.max(schemaMaxColUniqueId, column.getUniqueId()); + } + return schemaMaxColUniqueId; + } + public void initColumnNameMap() { // case insensitive nameToColumn = Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER); 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 index 8839fca5a54e1e..283e2f7f90b309 100644 --- 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 @@ -844,6 +844,43 @@ public void testAggAddOrDropColumn() throws Exception { } } + @Test + public void testReaddDroppedValueColumnUsesNewUniqueId() throws Exception { + String tableName = "sc_dup_readd_value_column"; + dropTable("test." + tableName, false); + createTable("CREATE TABLE test." + tableName + " (\n" + + "event_time DATETIME NOT NULL,\n" + + "user_id BIGINT NOT NULL,\n" + + "item_id INT NOT NULL,\n" + + "amount DECIMAL(10, 2) NOT NULL,\n" + + "city VARCHAR(64) NOT NULL\n" + + ") DUPLICATE KEY(event_time, user_id)\n" + + "DISTRIBUTED BY HASH(user_id) BUCKETS 1\n" + + "PROPERTIES ('replication_num' = '1', 'light_schema_change' = 'true')"); + + Database db = Env.getCurrentInternalCatalog().getDbOrMetaException("test"); + OlapTable tbl = (OlapTable) db.getTableOrMetaException(tableName, Table.TableType.OLAP); + MaterializedIndexMeta indexMeta = tbl.getIndexMetaByIndexId(tbl.getBaseIndexId()); + int droppedColumnUniqueId = tbl.getColumn("city").getUniqueId(); + + tbl.writeLock(); + try { + indexMeta.setMaxColUniqueId(Column.COLUMN_UNIQUE_ID_INIT_VALUE); + } finally { + tbl.writeUnlock(); + } + + alterTable("ALTER TABLE test." + tableName + " DROP COLUMN city", connectContext); + jobSize++; + waitAlterJobDone(Env.getCurrentEnv().getSchemaChangeHandler().getAlterJobsV2()); + Assertions.assertTrue(indexMeta.getMaxColUniqueId() >= droppedColumnUniqueId); + + alterTable("ALTER TABLE test." + tableName + " ADD COLUMN city VARCHAR(64)", connectContext); + jobSize++; + waitAlterJobDone(Env.getCurrentEnv().getSchemaChangeHandler().getAlterJobsV2()); + Assertions.assertTrue(tbl.getColumn("city").getUniqueId() > droppedColumnUniqueId); + } + @Test public void testUniqAddOrDropColumn() throws Exception { diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/MaterializedIndexMetaTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/MaterializedIndexMetaTest.java new file mode 100644 index 00000000000000..4e12aca4e3a5a3 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/MaterializedIndexMetaTest.java @@ -0,0 +1,64 @@ +// 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.catalog; + +import org.apache.doris.persist.gson.GsonUtils; +import org.apache.doris.thrift.TStorageType; + +import com.google.common.collect.Lists; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class MaterializedIndexMetaTest { + + @Test + public void testDeserializeRepairsStaleMaxColUniqueId() throws Exception { + MaterializedIndexMeta meta = createMeta(); + meta.setMaxColUniqueId(Column.COLUMN_UNIQUE_ID_INIT_VALUE); + + String json = GsonUtils.GSON.toJson(meta); + MaterializedIndexMeta replayedMeta = GsonUtils.GSON.fromJson(json, MaterializedIndexMeta.class); + + Assertions.assertEquals(7, replayedMeta.getMaxColUniqueId()); + replayedMeta.setSchema(Lists.newArrayList(replayedMeta.getSchema().get(0))); + Assertions.assertEquals(8, replayedMeta.incAndGetMaxColUniqueId()); + } + + @Test + public void testDeserializePreservesHigherMaxColUniqueId() throws Exception { + MaterializedIndexMeta meta = createMeta(); + meta.setMaxColUniqueId(11); + + String json = GsonUtils.GSON.toJson(meta); + MaterializedIndexMeta replayedMeta = GsonUtils.GSON.fromJson(json, MaterializedIndexMeta.class); + + replayedMeta.setSchema(Lists.newArrayList(replayedMeta.getSchema().get(0))); + Assertions.assertEquals(11, replayedMeta.getMaxColUniqueId()); + Assertions.assertEquals(12, replayedMeta.incAndGetMaxColUniqueId()); + } + + private MaterializedIndexMeta createMeta() { + Column keyColumn = new Column("k", PrimitiveType.INT); + keyColumn.setUniqueId(0); + Column valueColumn = new Column("v", PrimitiveType.INT); + valueColumn.setUniqueId(7); + return new MaterializedIndexMeta( + 1L, Lists.newArrayList(keyColumn, valueColumn), 1, 1, (short) 1, + TStorageType.COLUMN, KeysType.DUP_KEYS, null); + } +} diff --git a/regression-test/data/fault_injection_p0/test_readd_dropped_column_unique_id.out b/regression-test/data/fault_injection_p0/test_readd_dropped_column_unique_id.out new file mode 100644 index 00000000000000..e22313ec364596 --- /dev/null +++ b/regression-test/data/fault_injection_p0/test_readd_dropped_column_unique_id.out @@ -0,0 +1,8 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !old_row -- +1 \N + +-- !all_rows -- +1 \N +2 67890 + diff --git a/regression-test/suites/fault_injection_p0/test_readd_dropped_column_unique_id.groovy b/regression-test/suites/fault_injection_p0/test_readd_dropped_column_unique_id.groovy new file mode 100644 index 00000000000000..777e17ef682e9e --- /dev/null +++ b/regression-test/suites/fault_injection_p0/test_readd_dropped_column_unique_id.groovy @@ -0,0 +1,67 @@ +// 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. + +import org.apache.doris.regression.suite.ClusterOptions + +suite("test_readd_dropped_column_unique_id", "docker") { + def options = new ClusterOptions() + options.feConfigs += ["enable_debug_points=true"] + options.cloudMode = false + + docker(options) { + sql "DROP TABLE IF EXISTS test_readd_dropped_column_unique_id" + sql """ + CREATE TABLE test_readd_dropped_column_unique_id ( + k INT NOT NULL, + v INT NULL + ) + DUPLICATE KEY(k) + DISTRIBUTED BY HASH(k) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "light_schema_change" = "true", + "disable_auto_compaction" = "true" + ) + """ + sql "INSERT INTO test_readd_dropped_column_unique_id VALUES (1, 12345)" + sql "SYNC" + + def debugPoint = "FE.SchemaChangeHandler.updateBaseIndexSchema.forceStaleMaxColUniqueId" + try { + GetDebugPoint().enableDebugPointForAllFEs( + debugPoint, [table_name: "test_readd_dropped_column_unique_id"]) + sql "ALTER TABLE test_readd_dropped_column_unique_id DROP COLUMN v" + } finally { + GetDebugPoint().disableDebugPointForAllFEs(debugPoint) + } + + sql "ALTER TABLE test_readd_dropped_column_unique_id ADD COLUMN v INT NULL" + + order_qt_old_row """ + SELECT k, v + FROM test_readd_dropped_column_unique_id + ORDER BY k + """ + + sql "INSERT INTO test_readd_dropped_column_unique_id VALUES (2, 67890)" + order_qt_all_rows """ + SELECT k, v + FROM test_readd_dropped_column_unique_id + ORDER BY k + """ + } +}