mirror of
https://github.com/apache/doris.git
synced 2026-11-04 07:31:17 +00:00
[fix](fe) Run an ALTER that also needs heavy work as a heavy schema change instead of a light index change (#69444)
### What problem does this PR solve? Issue Number: None Related PR: #68674 Problem Summary: When an ALTER TABLE statement combines an index change that can run on the light index change path with a clause that needs a heavy schema change, the light path wins and the heavy work is never done: ```sql -- v was INT, idx is an INVERTED index: the shadow column __doris_shadow_v BIGINT is published and the -- schema change job is recorded as FINISHED, but no BE conversion job runs ALTER TABLE t MODIFY COLUMN v BIGINT NULL, DROP INDEX idx; ALTER TABLE t MODIFY COLUMN v BIGINT NULL, ADD INDEX i2 (x) USING INVERTED; -- enable_add_index_for_new_data = false: bf_v2 must be built for the existing data, but it is only -- published for new data ALTER TABLE t ADD INDEX bf_v2 (v2) USING BLOOMFILTER, DROP INDEX idx; ``` The same holds for every column or property clause that cannot run as a light schema change (dropping a key column, reordering columns, sequence mapping tables, table properties). On a changelog table such a statement also bypassed the check that only light changes are allowed, because that check looked at the candidate flag `lightIndexChange`. Root cause: in `SchemaChangeHandler#process` every clause that is not light sets `lightSchemaChange = false`, index clauses included, so nothing records that a clause needs a heavy schema change. The dispatch then takes the light index change branch whenever an index clause can be light. Fix: the clause loop also records `requiresHeavyChange` for column and property clauses that cannot be light and for an `ADD INDEX` that cannot be light. After the loop, the light index change path is only used when no clause needs heavy work; otherwise the statement runs as a heavy schema change job (`createJob`), which applies the index changes as well, exactly as it does when `enable_light_index_change` is off. The changelog check uses the path that is actually taken. No statement is newly rejected (other than on changelog tables, where the heavy path was never allowed). Notes: - A `DROP INDEX` without light support still needs no heavy work: in a light index change only its metadata is dropped (same-statement NGRAM_BF to BLOOMFILTER replacement in local mode keeps working). - The light DROP INDEX pre-check from #68674 still runs before the clause loop, because whether a clause needs heavy work is only known after it is processed. In cloud mode it therefore still requires a compute group for such a statement, as before this change.
This commit is contained in:
2 files changed
+84
-2
No files matched your search
@@ -2325,6 +2325,9 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
//alterClauses can or cannot light schema change
|
||||
boolean lightSchemaChange = true;
|
||||
boolean lightIndexChange = false;
|
||||
// A clause needs a heavy schema change job (a column or property change that can not be light, or
|
||||
// an index that has to be built for the existing data). The light index change path can not run it.
|
||||
boolean requiresHeavyChange = false;
|
||||
boolean buildIndexChange = false;
|
||||
|
||||
// index id -> index schema
|
||||
@@ -2508,10 +2511,12 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
indexSchemaMap, colUniqueIdSupplierMap);
|
||||
if (!clauseCanLightSchemaChange) {
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
// TODO sequence mapping do not support light schema yet
|
||||
if (olapTable.hasColumnSeqMapping() || !sequenceMapping.isEmpty()) {
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
} else if (alterOp instanceof AddColumnsOp) {
|
||||
// add columns
|
||||
@@ -2521,10 +2526,12 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
indexSchemaMap, false, colUniqueIdSupplierMap);
|
||||
if (!clauseCanLightSchemaChange) {
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
// TODO sequence mapping do not support light schema yet
|
||||
if (olapTable.hasColumnSeqMapping() || !sequenceMapping.isEmpty()) {
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
} else if (alterOp instanceof DropColumnOp) {
|
||||
// drop column and drop indexes on this column
|
||||
@@ -2533,10 +2540,12 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
indexSchemaMap, newIndexes);
|
||||
if (!clauseCanLightSchemaChange) {
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
// TODO sequence mapping do not support light schema yet
|
||||
if (olapTable.hasColumnSeqMapping() || !sequenceMapping.isEmpty()) {
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
} else if (alterOp instanceof ModifyColumnOp) {
|
||||
// modify column
|
||||
@@ -2544,15 +2553,18 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
olapTable, indexSchemaMap);
|
||||
if (!clauseCanLightSchemaChange) {
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
} else if (alterOp instanceof ReorderColumnsOp) {
|
||||
// reorder column
|
||||
processReorderColumn((ReorderColumnsOp) alterOp, olapTable, indexSchemaMap);
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
} else if (alterOp instanceof ModifyTablePropertiesOp) {
|
||||
// modify table properties
|
||||
// do nothing, properties are already in propertyMap
|
||||
lightSchemaChange = false;
|
||||
requiresHeavyChange = true;
|
||||
} else if (alterOp instanceof CreateIndexOp) {
|
||||
CreateIndexOp createIndexOp = (CreateIndexOp) alterOp;
|
||||
Index index = createIndexOp.getIndex();
|
||||
@@ -2567,6 +2579,9 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
alterIndexes.add(index);
|
||||
isDropIndex = false;
|
||||
lightIndexChange = true;
|
||||
} else {
|
||||
// The index has to be built for the existing data as well.
|
||||
requiresHeavyChange = true;
|
||||
}
|
||||
} else if (alterOp instanceof BuildIndexOp) {
|
||||
BuildIndexOp buildIndexOp = (BuildIndexOp) alterOp;
|
||||
@@ -2672,7 +2687,12 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
lightIndexChange, buildIndexChange, indexSchemaMap);
|
||||
}
|
||||
|
||||
if (olapTable.hasChangelog() && !(lightSchemaChange || lightIndexChange)) {
|
||||
// The light index change path only publishes the new schema and submits index delete jobs. When a
|
||||
// clause also needs a heavy schema change, the whole statement runs as a heavy schema change job,
|
||||
// which applies the index changes as well.
|
||||
boolean useLightIndexChange = Config.enable_light_index_change && lightIndexChange
|
||||
&& !requiresHeavyChange;
|
||||
if (olapTable.hasChangelog() && !(lightSchemaChange || useLightIndexChange)) {
|
||||
throw new DdlException("only support light schema change operator when use table with changelog");
|
||||
}
|
||||
|
||||
@@ -2681,7 +2701,7 @@ public class SchemaChangeHandler extends AlterHandler {
|
||||
//for schema change add/drop value column optimize, direct modify table meta.
|
||||
modifyTableLightSchemaChange(rawSql, db, olapTable, indexSchemaMap, newIndexes,
|
||||
null, isDropIndex, jobId, false, propertyMap);
|
||||
} else if (Config.enable_light_index_change && lightIndexChange) {
|
||||
} else if (useLightIndexChange) {
|
||||
Index.checkConflict(newIndexes, olapTable.getCopiedBfColumns());
|
||||
long jobId = Env.getCurrentEnv().getNextId();
|
||||
// For light index changes, directly modify table metadata first.
|
||||
|
||||
@@ -1485,6 +1485,68 @@ public class SchemaChangeHandlerTest extends TestWithFeService {
|
||||
tbl.getIndexes().stream().map(Index::getIndexName).collect(Collectors.toList()));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testLightIndexChangeWithHeavyWorkRunsHeavySchemaChange() throws Exception {
|
||||
String tableName = "sc_light_index_with_heavy_work";
|
||||
dropTable("test." + tableName, false);
|
||||
createTable("CREATE TABLE test." + tableName + " (\n"
|
||||
+ "k1 INT,\n"
|
||||
+ "vi INT,\n"
|
||||
+ "v1 STRING,\n"
|
||||
+ "v2 STRING,\n"
|
||||
+ "v3 STRING,\n"
|
||||
+ "INDEX idx_v1 (v1) USING INVERTED,\n"
|
||||
+ "INDEX idx_v3 (v3) USING INVERTED\n"
|
||||
+ ") ENGINE=OLAP\n"
|
||||
+ "DUPLICATE KEY(k1)\n"
|
||||
+ "DISTRIBUTED BY HASH(k1) 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);
|
||||
SchemaChangeHandler handler = Env.getCurrentEnv().getSchemaChangeHandler();
|
||||
String alterPrefix = "ALTER TABLE test." + tableName + " ";
|
||||
|
||||
// A type change needs a BE conversion job, so the light DROP INDEX must not publish the shadow schema.
|
||||
assertRunsHeavySchemaChange(handler, tbl, alterPrefix + "MODIFY COLUMN vi BIGINT NULL, DROP INDEX idx_v1");
|
||||
Assertions.assertEquals(PrimitiveType.BIGINT, tbl.getColumn("vi").getType().getPrimitiveType());
|
||||
Assertions.assertTrue(tbl.getBaseSchema(true).stream()
|
||||
.noneMatch(column -> column.getName().startsWith(SchemaChangeHandler.SHADOW_NAME_PREFIX)));
|
||||
Assertions.assertTrue(tbl.getIndexes().stream().noneMatch(idx -> idx.getIndexName().equals("idx_v1")));
|
||||
|
||||
// Without new-data-only indexing, the BLOOMFILTER index has to be built for the existing data too.
|
||||
boolean enableAddIndexForNewData = connectContext.getSessionVariable().isEnableAddIndexForNewData();
|
||||
connectContext.getSessionVariable().setEnableAddIndexForNewData(false);
|
||||
try {
|
||||
assertRunsHeavySchemaChange(handler, tbl,
|
||||
alterPrefix + "ADD INDEX bf_v2 (v2) USING BLOOMFILTER, DROP INDEX idx_v3");
|
||||
} finally {
|
||||
connectContext.getSessionVariable().setEnableAddIndexForNewData(enableAddIndexForNewData);
|
||||
}
|
||||
Assertions.assertEquals(Lists.newArrayList("bf_v2"),
|
||||
tbl.getIndexes().stream().map(Index::getIndexName).collect(Collectors.toList()));
|
||||
}
|
||||
|
||||
// Runs the statement and checks it ran as a heavy schema change job, not as a light index change that
|
||||
// publishes the schema at once and submits index delete jobs.
|
||||
private void assertRunsHeavySchemaChange(SchemaChangeHandler handler, OlapTable tbl, String sql)
|
||||
throws Exception {
|
||||
Set<Long> alterJobIdsBefore = Sets.newHashSet(handler.getAlterJobsV2().keySet());
|
||||
long indexChangeJobsOfTable = handler.getIndexChangeJobs().values().stream()
|
||||
.filter(job -> job.getTableId() == tbl.getId()).count();
|
||||
|
||||
alterTable(sql, connectContext);
|
||||
jobSize++;
|
||||
|
||||
List<AlterJobV2> newJobs = handler.getAlterJobsV2().values().stream()
|
||||
.filter(job -> job.getTableId() == tbl.getId() && !alterJobIdsBefore.contains(job.getJobId()))
|
||||
.collect(Collectors.toList());
|
||||
Assertions.assertEquals(1, newJobs.size());
|
||||
Assertions.assertEquals(indexChangeJobsOfTable, handler.getIndexChangeJobs().values().stream()
|
||||
.filter(job -> job.getTableId() == tbl.getId()).count());
|
||||
waitAlterJobDone(handler.getAlterJobsV2());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testAddInvertedIndexStoresCanonicalBuiltinAnalyzer() throws Exception {
|
||||
createAnalyzerAliasTable("sc_ik_alias");
|
||||
|
||||
Reference in new issue
Block a user