github-actions[bot] commented on code in PR #70352:
URL: https://github.com/apache/doris/pull/70352#discussion_r4492211754
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2179,37 +2201,65 @@ public int getAsInt() {
buildIndexChange = true;
lightSchemaChange = false;
} else if (alterClause instanceof DropIndexClause) {
- if (processDropIndex((DropIndexClause) alterClause,
olapTable, newIndexes)) {
- return;
- }
- lightSchemaChange = false;
-
DropIndexClause dropIndexClause = (DropIndexClause)
alterClause;
- List<Index> existedIndexes = olapTable.getIndexes();
- Index found = null;
- for (Index existedIdx : existedIndexes) {
- if
(existedIdx.getIndexName().equalsIgnoreCase(dropIndexClause.getIndexName())) {
- found = existedIdx;
- break;
+
+ if (dropIndexClause.hasPartitionSpec()) {
+ // Delete the physical index files and keep the
table-level index definition.
+ List<Index> existedIndexes = olapTable.getIndexes();
+ Index found = null;
+ for (Index existedIdx : existedIndexes) {
+ if
(existedIdx.getIndexName().equalsIgnoreCase(dropIndexClause.getIndexName())) {
+ found = existedIdx;
+ break;
+ }
}
- }
- // for inverted index, light schema change is supported in
both cloud and local mode;
- // for ngram index, light schema change is supported only
in cloud mode;
- boolean supportLightIndexChange = false;
- if (Config.isCloudMode()) {
- if (enableAddIndexForNewData) {
- supportLightIndexChange = (
- found.getIndexType() == IndexType.NGRAM_BF
- || found.getIndexType() ==
IndexDef.IndexType.INVERTED);
+ if (found == null) {
+ if (dropIndexClause.isSetIfExists()) {
+ LOG.info("drop index[{}] which does not exist
on table[{}]",
+ dropIndexClause.getIndexName(),
olapTable.getName());
+ return;
+ }
+ throw new DdlException("index " +
dropIndexClause.getIndexName() + " does not exist");
}
- } else {
- supportLightIndexChange = found.getIndexType() ==
IndexDef.IndexType.INVERTED
- || found.getIndexType() ==
IndexDef.IndexType.ANN;
- }
- if (found != null && supportLightIndexChange) {
+ if (found.getIndexType() != IndexType.INVERTED) {
+ throw new DdlException(
+ "Only inverted index supports DROP INDEX
ON PARTITION");
+ }
+ if (!olapTable.isPartitionedTable()) {
+ throw new DdlException("table " +
olapTable.getName()
+ + " is not partitioned, cannot drop index
with partitions");
+ }
+ Set<String> specifiedPartitions = new
HashSet<>(dropIndexClause.getPartitionNames());
+ for (String partName : specifiedPartitions) {
+ if (olapTable.getPartition(partName) == null) {
+ throw new DdlException("partition " + partName
+ " does not exist");
+ }
+ }
+
alterIndexes.add(found);
+ indexOnPartitions.put(found.getIndexId(),
specifiedPartitions);
isDropIndex = true;
- lightIndexChange = true;
+ buildIndexChange = true;
Review Comment:
[P2] Reject partition DROP when light index changes are disabled
With the mutable FE setting `enable_light_index_change=false`, this new
branch sets `buildIndexChange=true` and `lightSchemaChange=false`, but never
enables `lightIndexChange`. Dispatch reaches the `buildIndexChange` branch
below, whose only operation is guarded by `Config.enable_light_index_change`,
and then returns success without creating a job or deleting anything. The
parser/op/clause has no upstream rejection for this setting. Reject the
statement when its required execution mode is disabled, or route it to an
implementation that performs the requested deletion.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2179,37 +2201,65 @@ public int getAsInt() {
buildIndexChange = true;
lightSchemaChange = false;
} else if (alterClause instanceof DropIndexClause) {
- if (processDropIndex((DropIndexClause) alterClause,
olapTable, newIndexes)) {
- return;
- }
- lightSchemaChange = false;
-
DropIndexClause dropIndexClause = (DropIndexClause)
alterClause;
- List<Index> existedIndexes = olapTable.getIndexes();
- Index found = null;
- for (Index existedIdx : existedIndexes) {
- if
(existedIdx.getIndexName().equalsIgnoreCase(dropIndexClause.getIndexName())) {
- found = existedIdx;
- break;
+
+ if (dropIndexClause.hasPartitionSpec()) {
+ // Delete the physical index files and keep the
table-level index definition.
+ List<Index> existedIndexes = olapTable.getIndexes();
+ Index found = null;
+ for (Index existedIdx : existedIndexes) {
+ if
(existedIdx.getIndexName().equalsIgnoreCase(dropIndexClause.getIndexName())) {
+ found = existedIdx;
+ break;
+ }
}
- }
- // for inverted index, light schema change is supported in
both cloud and local mode;
- // for ngram index, light schema change is supported only
in cloud mode;
- boolean supportLightIndexChange = false;
- if (Config.isCloudMode()) {
- if (enableAddIndexForNewData) {
- supportLightIndexChange = (
- found.getIndexType() == IndexType.NGRAM_BF
- || found.getIndexType() ==
IndexDef.IndexType.INVERTED);
+ if (found == null) {
+ if (dropIndexClause.isSetIfExists()) {
+ LOG.info("drop index[{}] which does not exist
on table[{}]",
+ dropIndexClause.getIndexName(),
olapTable.getName());
+ return;
+ }
+ throw new DdlException("index " +
dropIndexClause.getIndexName() + " does not exist");
}
- } else {
- supportLightIndexChange = found.getIndexType() ==
IndexDef.IndexType.INVERTED
- || found.getIndexType() ==
IndexDef.IndexType.ANN;
- }
- if (found != null && supportLightIndexChange) {
+ if (found.getIndexType() != IndexType.INVERTED) {
+ throw new DdlException(
+ "Only inverted index supports DROP INDEX
ON PARTITION");
+ }
+ if (!olapTable.isPartitionedTable()) {
+ throw new DdlException("table " +
olapTable.getName()
+ + " is not partitioned, cannot drop index
with partitions");
+ }
+ Set<String> specifiedPartitions = new
HashSet<>(dropIndexClause.getPartitionNames());
+ for (String partName : specifiedPartitions) {
+ if (olapTable.getPartition(partName) == null) {
+ throw new DdlException("partition " + partName
+ " does not exist");
+ }
+ }
+
alterIndexes.add(found);
+ indexOnPartitions.put(found.getIndexId(),
specifiedPartitions);
Review Comment:
[P1] Reject cloud partition drops until the backend can honor them
For a cloud table whose rowsets were written at the current schema version,
`DROP INDEX idx ON t PARTITION (p1)` reaches
`buildOrDeleteTableInvertedIndices` with the unchanged schema version and full
index list, including `idx`. `EngineCloudIndexChangeTask` ignores
`is_drop_op`/`alter_inverted_indexes`, and
`CloudTablet::pick_a_rowset_for_index_change` skips rowsets already at that
version. The job therefore reaches FINISHED without removing the requested
index; older rowsets are rebuilt with the unchanged list and retain it too.
This needs a cloud physical-drop implementation with appropriate rowset
selection, or an explicit cloud-mode rejection before jobs are created.
##########
regression-test/suites/inverted_index_p0/index_change/test_drop_index_on_partition.groovy:
##########
@@ -0,0 +1,278 @@
+// 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.
+
+
+suite("test_drop_index_on_partition", "inverted_index") {
+ def timeout = 300000
+ def delta_time = 1000
+
+ def get_build_index_job_count = { table_name ->
+ def res = sql """SHOW BUILD INDEX WHERE TableName = "${table_name}";"""
+ return res.size()
+ }
+
+ def wait_for_build_index_on_partition_finish = { table_name,
expected_job_count, OpTimeout ->
+ def useTime = 0
+ for (int t = delta_time; t <= OpTimeout; t += delta_time) {
+ def alter_res = sql """SHOW BUILD INDEX WHERE TableName =
"${table_name}";"""
+ if (alter_res.size() < expected_job_count) {
+ useTime = t
+ sleep(delta_time)
+ continue
+ }
+ def finished_num = 0
+ for (int i = 0; i < alter_res.size(); i++) {
+ logger.info(table_name + " build index job state: " +
alter_res[i][7] + " idx: " + i)
+ if (alter_res[i][7] == "FINISHED") {
+ ++finished_num
+ }
+ }
+ if (finished_num == alter_res.size()) {
+ logger.info(table_name + " all build index jobs finished,
detail: " + alter_res)
+ break
+ }
+ useTime = t
+ sleep(delta_time)
+ }
+ assertTrue(useTime <= OpTimeout,
"wait_for_build_index_on_partition_finish timeout")
Review Comment:
[P2] Fail the wait helper when jobs never reach FINISHED
Every assignment to `useTime` uses `t` from a loop bounded by `t <=
OpTimeout`, so this assertion is still true after all polls are exhausted. A
CANCELLED or permanently RUNNING DROP job waits five minutes and is then
treated as successful. The following checks only verify retained index metadata
and fallback/unfiltered query results, which also pass when physical deletion
never occurs. Track actual completion and assert it after the loop, failing on
cancellation or timeout, so these new regression cases can detect failed DROP
jobs.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -3438,25 +3492,86 @@ public void buildOrDeleteTableInvertedIndices(Database
db, OlapTable olapTable,
}
}
+ // Inverted index supports light schema change in both cloud and local
mode.
+ // NGRAM_BF supports it only in cloud mode.
+ private static boolean isLightDropIndexSupported(Index index, boolean
enableAddIndexForNewData) {
+ if (Config.isCloudMode()) {
+ return enableAddIndexForNewData
+ && (index.getIndexType() == IndexType.NGRAM_BF
+ || index.getIndexType() == IndexType.INVERTED);
+ }
+ return index.getIndexType() == IndexType.INVERTED ||
index.getIndexType() == IndexType.ANN;
+ }
+
+ // Reject conflicting jobs and mixed add/drop operations before publishing
index metadata.
+ private void checkLightDropIndexPreconditions(Database db, OlapTable
olapTable, List<AlterClause> alterClauses,
+ boolean enableAddIndexForNewData) throws UserException {
+ if (!Config.enable_light_index_change) {
+ return;
+ }
+ List<Index> droppedIndexes = new ArrayList<>();
+ for (AlterClause alterClause : alterClauses) {
+ if (!(alterClause instanceof DropIndexClause)
+ || ((DropIndexClause) alterClause).hasPartitionSpec()) {
Review Comment:
[P2] Preflight all selected partitions before publishing DROP jobs
Skipping partition-scoped drops here leaves their conflict checks inside the
publication loop. For `DROP INDEX idx ON t PARTITIONS (a,b)`, if `a` is visited
first and `b` has an unfinished BUILD of `idx`, the loop adds `a`'s DROP to
both job maps and journals it before discovering `b`'s conflict and throwing.
The catch only rethrows, so the SQL reports failure while `a`'s deletion
remains runnable and survives replay after failover. Validate conflicts across
the complete selected partition set under the table write lock before the first
job is registered or logged.
--
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]
