github-actions[bot] commented on code in PR #68674:
URL: https://github.com/apache/doris/pull/68674#discussion_r4162451511
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3529,6 +3529,11 @@ public void modifyTableLightSchemaChange(String rawSql,
Database db, OlapTable o
currentIndexMeta.getSchemaHash(),
currentIndexMeta.getShortKeyColumnCount(), entry.getValue());
}
+ // Reject the drop before touching metadata, so a failed DROP INDEX
leaves the table unchanged.
Review Comment:
[P1] Validate the cloud compute group before changing index metadata. A
cloud user can have table ALTER privilege but no usable compute group. This
preflight passes, then `updateBaseIndexSchema` removes the index and the edit
log records that change; `buildOrDeleteTableInvertedIndices` calls
`getCloudClusterName()` afterward and throws, so DROP INDEX reports failure
with the index already gone and no delete job. Resolve the compute group before
the metadata and journal writes, and cover this failure with a
metadata-preservation test.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3529,6 +3529,11 @@ public void modifyTableLightSchemaChange(String rawSql,
Database db, OlapTable o
currentIndexMeta.getSchemaHash(),
currentIndexMeta.getShortKeyColumnCount(), entry.getValue());
}
+ // Reject the drop before touching metadata, so a failed DROP INDEX
leaves the table unchanged.
+ if (alterIndexes != null && isDropIndex && !isReplay) {
+ checkNoIndexChangeJobOnPartitions(db, olapTable, alterIndexes,
olapTable.getPartitionNames());
+ }
Review Comment:
[P2] Run this conflict check before clauses mutate live table metadata. In
an allowed `DROP COLUMN b, DROP INDEX idx` batch, `processDropColumn` first
calls `olapTable.setBloomFilterInfo` and removes b from the table's
bloom-filter setting. If an unfinished partition-scoped DROP job for idx
exists, this new guard then throws before `updateBaseIndexSchema` or the edit
log. The failed ALTER leaves column b and idx intact but silently loses b's
bloom-filter setting in memory. Stage that change until the preflight passes,
or check the conflict before processing such clauses.
##########
fe/fe-core/src/test/java/org/apache/doris/alter/SchemaChangeHandlerTest.java:
##########
@@ -1233,6 +1233,36 @@ public void testAddDuplicateInvertedIndexException()
throws Exception {
}
}
+ @Test
+ public void testCheckNoIndexChangeJobOnPartitions() throws Exception {
+ Database db =
Env.getCurrentInternalCatalog().getDbOrMetaException("test");
+ OlapTable tbl = (OlapTable) db.getTableOrMetaException("sc_dup",
Table.TableType.OLAP);
+ SchemaChangeHandler handler =
Env.getCurrentEnv().getSchemaChangeHandler();
+ List<Index> alterIndexes = Lists.newArrayList(
+ new Index(1L, "idx_error_msg",
Lists.newArrayList("error_msg"), IndexType.NGRAM_BF, null, ""));
+
+ handler.checkNoIndexChangeJobOnPartitions(db, tbl, alterIndexes,
tbl.getPartitionNames());
+
+ long jobId = Env.getCurrentEnv().getNextId();
+ IndexChangeJob job = new IndexChangeJob(jobId, db.getId(),
tbl.getId(), tbl.getName(), 1000L, 0,
+ Lists.newArrayList(), Lists.newArrayList());
+ job.setOriginIndexId(tbl.getBaseIndexId());
+ job.setPartitionName(tbl.getPartitionNames().iterator().next());
+ job.setAlterInvertedIndexInfo(true, alterIndexes);
+ handler.addIndexChangeJob(job);
Review Comment:
[P2] Keep this synthetic job out of the live scheduler. `addIndexChangeJob`
also queues it in `runnableIndexChangeJob`; a scheduler tick can cancel it
before the following `assertThrows` (the fake job has a 1 s timeout and no
partition ID). Since the conflict predicate ignores finished or cancelled jobs,
the test can fail depending on timing. Put the fake job only in
`indexChangeJobs`, or control the scheduler while asserting its state.
##########
fe/fe-core/src/test/java/org/apache/doris/alter/SchemaChangeHandlerTest.java:
##########
@@ -1233,6 +1233,36 @@ public void testAddDuplicateInvertedIndexException()
throws Exception {
}
}
+ @Test
+ public void testCheckNoIndexChangeJobOnPartitions() throws Exception {
Review Comment:
[P2] Test the rejected DROP INDEX command and its metadata. This test
invokes the helper directly with an index absent from `sc_dup`, so it passes
even if the new call before `updateBaseIndexSchema` is removed or moved back
after the metadata update. Local NGRAM_BF DROP also takes the heavy path. Use a
real light-change INVERTED index, register an unfinished conflicting job,
execute `ALTER TABLE ... DROP INDEX`, then assert the error and unchanged index
list and base-index schema version.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3891,6 +3896,18 @@ public void buildOrDeleteTableInvertedIndices(Database
db, OlapTable olapTable,
}
}
+ // Throws if any of the partitions still has an unfinished index change
job on the base index.
+ void checkNoIndexChangeJobOnPartitions(Database db, OlapTable olapTable,
List<Index> alterIndexes,
+ Set<String> partitionNames) throws DdlException {
Review Comment:
[P2] Scan the job map once for all partitions. This loop calls
`hasIndexChangeJobOnPartition` for every partition, and each call copies/scans
the entire global `indexChangeJobs` history. A recent build on a P-partition
table leaves at least P retained job records for seven days, so the new guard
adds O(P²) copying and comparisons under the table write lock, followed by the
existing per-partition job check. Collect matching unfinished partition names
from one snapshot before this loop.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3529,6 +3529,11 @@ public void modifyTableLightSchemaChange(String rawSql,
Database db, OlapTable o
currentIndexMeta.getSchemaHash(),
currentIndexMeta.getShortKeyColumnCount(), entry.getValue());
}
+ // Reject the drop before touching metadata, so a failed DROP INDEX
leaves the table unchanged.
+ if (alterIndexes != null && isDropIndex && !isReplay) {
+ checkNoIndexChangeJobOnPartitions(db, olapTable, alterIndexes,
olapTable.getPartitionNames());
Review Comment:
[P1] Keep heavy column changes out of this light DROP path. A batched
`MODIFY COLUMN v BIGINT, DROP INDEX idx` is allowed; the type change marks
`lightSchemaChange=false` and creates a shadow column, but DROP sets
`lightIndexChange=true`, so dispatch calls this method. It publishes the
shadow/BIGINT schema and marks a schema job FINISHED without creating the
required BE conversion job. Reject this combination or route it through a
correct heavy schema change before publishing metadata.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3891,6 +3896,18 @@ public void buildOrDeleteTableInvertedIndices(Database
db, OlapTable olapTable,
}
}
+ // Throws if any of the partitions still has an unfinished index change
job on the base index.
+ void checkNoIndexChangeJobOnPartitions(Database db, OlapTable olapTable,
List<Index> alterIndexes,
+ Set<String> partitionNames) throws DdlException {
+ for (String partitionName : partitionNames) {
Review Comment:
[P2] Preserve partition targets for mixed DROP INDEX clauses. `DROP INDEX
idx1 ON PARTITION p1, DROP INDEX idx2` is allowed, and the full drop makes this
light path win. The scoped idx1 remains in table metadata, but this method
receives no `indexOnPartitions` map; its preflight and later delete-job map
apply idx1 to every partition. BE therefore removes idx1 files outside p1 even
though FE still advertises idx1 there. Carry per-index partition targets
through this path or reject the mixed-scope batch.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3891,6 +3896,18 @@ public void buildOrDeleteTableInvertedIndices(Database
db, OlapTable olapTable,
}
}
+ // Throws if any of the partitions still has an unfinished index change
job on the base index.
+ void checkNoIndexChangeJobOnPartitions(Database db, OlapTable olapTable,
List<Index> alterIndexes,
+ Set<String> partitionNames) throws DdlException {
+ for (String partitionName : partitionNames) {
+ if (hasIndexChangeJobOnPartition(olapTable.getBaseIndexId(),
db.getId(), olapTable.getId(),
Review Comment:
[P2] Treat a pending BUILD of the same index as a DROP conflict. In local
mode this call passes `isDrop=true`, but `hasSameAlterInvertedIndex` ignores
the unfinished `BUILD INDEX idx` job because its direction is false. FE can run
the old BUILD and new DROP jobs in either order. If the delete task reaches BE
first, it sees no index-bearing rowsets and finishes; the older build then adds
`idx` to rowset schemas after FE has removed it from table metadata. Reject or
cancel the pending build before accepting DROP, or enforce a durable job order.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3529,6 +3529,11 @@ public void modifyTableLightSchemaChange(String rawSql,
Database db, OlapTable o
currentIndexMeta.getSchemaHash(),
currentIndexMeta.getShortKeyColumnCount(), entry.getValue());
}
+ // Reject the drop before touching metadata, so a failed DROP INDEX
leaves the table unchanged.
+ if (alterIndexes != null && isDropIndex && !isReplay) {
Review Comment:
[P1] Preserve DROP INDEX work in mixed index batches. `DROP INDEX old_idx,
ADD INDEX new_idx` is accepted, but the ADD resets the shared `isDropIndex`
flag to false. This guard is skipped, `updateBaseIndexSchema` removes
`old_idx`, and the later delete-job branch is skipped, leaving its physical
index files behind. Reversing clause order sends delete jobs for both old and
new IDs. Track additions and drops separately or reject mixed batches before
changing metadata.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
