This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 713dad1a3f [#12827] fix(doris): handle missing index deletes (#12828)
713dad1a3f is described below
commit 713dad1a3f06ae71a99c484e95584989fa86a539
Author: StormSpirit <[email protected]>
AuthorDate: Fri Sep 18 23:19:19 2026 +0800
[#12827] fix(doris): handle missing index deletes (#12828)
### What changes were proposed in this pull request?
Update the Doris JDBC catalog so `TableChange.deleteIndex(name, true)`
treats a missing index as a no-op without emitting a DROP fragment,
while preserving strict missing-index validation and existing-index
deletion.
Validate index changes before metadata loading or SQL generation so
duplicate DeleteIndex changes and same-name AddIndex/DeleteIndex changes
fail fast in either request order with deterministic errors.
Filter empty fragments from the combined Doris `ALTER TABLE` statement
and add unit, operation, and Doris 3.x/4.x integration coverage for the
missing/existing index matrix, mixed no-op requests, conflict requests,
and missing-table error propagation.
### Why are the changes needed?
The Doris implementation currently skips its local existence check when
`ifExists=true` but still generates `DROP INDEX` inside the combined
`ALTER TABLE` statement. Doris then rejects a missing index, which
violates the public `TableChange.DeleteIndex` contract and prevents
idempotent cleanup operations.
The change keeps the connector's existing batched ALTER model and does
not assume undocumented `ALTER TABLE ... DROP INDEX IF EXISTS` syntax.
It also prevents a no-op delete from masking same-name conflicting index
changes or unrelated validation failures.
Fix: #12827
### Does this PR introduce _any_ user-facing change?
Yes. Deleting a missing Doris index with `ifExists=true` now succeeds as
a no-op. Existing-index deletion and `ifExists=false` strict behavior
remain unchanged. Duplicate deletes and same-name AddIndex/DeleteIndex
requests are rejected before DDL execution. No public API, OpenAPI
field, or property key is added or removed.
### How was this patch tested?
- `./gradlew :catalogs:catalog-jdbc-doris:test -PskipITs` — passed; 38
tests, 0 skipped, 0 failures, 0 errors.
- `./gradlew :catalogs:catalog-jdbc-doris:test --tests
"org.apache.gravitino.catalog.doris.operation.TestDorisTableOperations"
-PskipDockerTests=false` — passed; 9 tests, 0 skipped, 0 failures, 0
errors.
- `env NEED_CREATE_DOCKER_NETWORK=false ./gradlew
:catalogs:catalog-jdbc-doris:test --tests
"org.apache.gravitino.catalog.doris.integration.test.CatalogDoris3xIT"
-PskipDockerTests=false -PdorisMultiVersionTest` — passed on Doris
3.0.6.2; 15 tests, 0 skipped, 0 failures, 0 errors.
- `env NEED_CREATE_DOCKER_NETWORK=false ./gradlew
:catalogs:catalog-jdbc-doris:test --tests
"org.apache.gravitino.catalog.doris.integration.test.CatalogDoris4xIT"
-PskipDockerTests=false -PdorisMultiVersionTest` — passed on Doris
4.0.6; 15 tests, 0 skipped, 0 failures, 0 errors.
- `./gradlew :catalogs:catalog-jdbc-doris:spotlessCheck` — passed.
- `./gradlew rat` — passed.
- `./gradlew :catalogs:catalog-jdbc-doris:build -x test` — passed.
- The local-only Gravitino precheck passed.
- The first Doris 3.x attempt was blocked during test-network
initialization by an unrelated active endpoint; the recovery run used
the isolated network environment shown above and passed.
---------
Signed-off-by: jiangxt2 <[email protected]>
---
.../doris/operation/DorisTableOperations.java | 48 ++++++-
.../doris/integration/test/CatalogDoris3xIT.java | 133 +++++++++++++++++++
.../doris/integration/test/CatalogDoris4xIT.java | 132 +++++++++++++++++++
.../doris/operation/TestDorisTableOperations.java | 12 +-
.../TestDorisTableOperationsSqlGeneration.java | 141 ++++++++++++++++++++-
5 files changed, 451 insertions(+), 15 deletions(-)
diff --git
a/catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java
b/catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java
index af8e19749e..40b31f3588 100644
---
a/catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java
+++
b/catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java
@@ -39,6 +39,7 @@ import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
@@ -746,6 +747,7 @@ public class DorisTableOperations extends
JdbcTableOperations {
* */
// Not all operations require the original table information, so lazy
loading is used here
+ validateIndexChangeConflicts(changes);
JdbcTable lazyLoadTable = null;
TableChange.UpdateComment updateComment = null;
List<TableChange.SetProperty> setProperties = new ArrayList<>();
@@ -819,15 +821,44 @@ public class DorisTableOperations extends
JdbcTableOperations {
alterSql.add("MODIFY COMMENT \"" + escapeSqlLiteral(newComment, '"') +
"\"");
}
- if (CollectionUtils.isEmpty(alterSql)) {
+ List<String> nonEmptyAlterSql =
+
alterSql.stream().filter(StringUtils::isNotEmpty).collect(Collectors.toList());
+ if (CollectionUtils.isEmpty(nonEmptyAlterSql)) {
return "";
}
// Return the generated SQL statement
- String result = "ALTER TABLE `" + tableName + "`\n" + String.join(",\n",
alterSql) + ";";
+ String result =
+ "ALTER TABLE `" + tableName + "`\n" + String.join(",\n",
nonEmptyAlterSql) + ";";
LOG.info("Generated alter table:{}.{} sql: {}", databaseName, tableName,
result);
return result;
}
+ private static void validateIndexChangeConflicts(TableChange... changes) {
+ Set<String> deleteIndexNames = new HashSet<>();
+ Set<String> addIndexNames = new HashSet<>();
+
+ for (TableChange change : changes) {
+ if (change instanceof TableChange.DeleteIndex) {
+ String indexName = ((TableChange.DeleteIndex) change).getName();
+ Preconditions.checkArgument(
+ deleteIndexNames.add(indexName),
+ "Index '%s' cannot be deleted more than once in the same request",
+ indexName);
+ Preconditions.checkArgument(
+ !addIndexNames.contains(indexName),
+ "Index '%s' cannot be added and deleted in the same request",
+ indexName);
+ } else if (change instanceof TableChange.AddIndex) {
+ String indexName = ((TableChange.AddIndex) change).getName();
+ Preconditions.checkArgument(
+ !deleteIndexNames.contains(indexName),
+ "Index '%s' cannot be added and deleted in the same request",
+ indexName);
+ addIndexNames.add(indexName);
+ }
+ }
+ }
+
private String updateColumnNullabilityDefinition(
TableChange.UpdateColumnNullability change, JdbcTable table) {
validateUpdateColumnNullable(change, table);
@@ -1035,11 +1066,14 @@ public class DorisTableOperations extends
JdbcTableOperations {
static String deleteIndexDefinition(
JdbcTable lazyLoadTable, TableChange.DeleteIndex deleteIndex) {
- if (!deleteIndex.isIfExists()) {
- Preconditions.checkArgument(
- Arrays.stream(lazyLoadTable.index())
- .anyMatch(index -> index.name().equals(deleteIndex.getName())),
- "Index does not exist");
+ boolean indexExists =
+ Arrays.stream(lazyLoadTable.index())
+ .anyMatch(index -> index.name().equals(deleteIndex.getName()));
+ if (!indexExists) {
+ if (deleteIndex.isIfExists()) {
+ return "";
+ }
+ throw new IllegalArgumentException("Index does not exist: " +
deleteIndex.getName());
}
return "DROP INDEX `" + deleteIndex.getName() + "`";
}
diff --git
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris3xIT.java
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris3xIT.java
index 97da5a1c03..8c5a6ba0ba 100644
---
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris3xIT.java
+++
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris3xIT.java
@@ -26,6 +26,7 @@ import static
org.apache.gravitino.integration.test.util.ITUtils.assertPartition
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import com.google.common.collect.Maps;
@@ -45,6 +46,7 @@ import org.apache.gravitino.Catalog;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.NoSuchTableException;
import org.apache.gravitino.integration.test.container.ContainerSuite;
import org.apache.gravitino.integration.test.container.DorisContainer;
import org.apache.gravitino.integration.test.container.DorisImageName;
@@ -305,6 +307,137 @@ public class CatalogDoris3xIT extends BaseIT {
.untilAsserted(() -> assertEquals(0,
tc.loadTable(tid).index().length));
}
+ @Test
+ void testDeleteMissingIndexIfExists() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier tid =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_missing_idx"));
+
+ tc.createTable(
+ tid,
+ basicColumns(),
+ tableComment,
+ Collections.emptyMap(),
+ Transforms.EMPTY_TRANSFORM,
+ hashDist(),
+ null,
+ null);
+
+ tc.alterTable(tid, TableChange.deleteIndex("idx_missing", true));
+ assertEquals(0, tc.loadTable(tid).index().length);
+
+ IllegalArgumentException exception =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> tc.alterTable(tid, TableChange.deleteIndex("idx_missing",
false)));
+ assertTrue(exception.getMessage().contains("Index does not exist"),
exception.getMessage());
+ }
+
+ @Test
+ void testDeleteExistingInvertedIndexWithIfExistsFalse() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier tid =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_existing_idx_strict"));
+
+ tc.createTable(
+ tid,
+ basicColumns(),
+ tableComment,
+ Collections.emptyMap(),
+ Transforms.EMPTY_TRANSFORM,
+ hashDist(),
+ null,
+ null);
+ tc.alterTable(
+ tid,
+ TableChange.addIndex(Index.IndexType.INVERTED, "idx_strict", new
String[][] {{colName2}}));
+
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(1,
tc.loadTable(tid).index().length));
+
+ tc.alterTable(tid, TableChange.deleteIndex("idx_strict", false));
+
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(0,
tc.loadTable(tid).index().length));
+ }
+
+ @Test
+ void testNoOpDeleteComposesWithUnrelatedChanges() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier addColumnFirst =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_column_first"));
+ NameIdentifier addColumnLast =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_column_last"));
+ NameIdentifier addIndexFirst =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_index_first"));
+ NameIdentifier addIndexLast =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_index_last"));
+
+ for (NameIdentifier tid :
+ new NameIdentifier[] {addColumnFirst, addColumnLast, addIndexFirst,
addIndexLast}) {
+ tc.createTable(
+ tid,
+ basicColumns(),
+ tableComment,
+ Collections.emptyMap(),
+ Transforms.EMPTY_TRANSFORM,
+ hashDist(),
+ null,
+ null);
+ }
+
+ tc.alterTable(
+ addColumnFirst,
+ TableChange.deleteIndex("idx_missing", true),
+ TableChange.addColumn(new String[] {"col_extra"},
Types.VarCharType.of(100)));
+ tc.alterTable(
+ addColumnLast,
+ TableChange.addColumn(new String[] {"col_extra"},
Types.VarCharType.of(100)),
+ TableChange.deleteIndex("idx_missing", true));
+
+ for (NameIdentifier tid : new NameIdentifier[] {addColumnFirst,
addColumnLast}) {
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(3,
tc.loadTable(tid).columns().length));
+ }
+
+ tc.alterTable(
+ addIndexFirst,
+ TableChange.deleteIndex("idx_missing", true),
+ TableChange.addIndex(
+ Index.IndexType.INVERTED, "idx_unrelated", new String[][]
{{colName2}}));
+ tc.alterTable(
+ addIndexLast,
+ TableChange.addIndex(
+ Index.IndexType.INVERTED, "idx_unrelated", new String[][]
{{colName2}}),
+ TableChange.deleteIndex("idx_missing", true));
+
+ for (NameIdentifier tid : new NameIdentifier[] {addIndexFirst,
addIndexLast}) {
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(1,
tc.loadTable(tid).index().length));
+ }
+ }
+
+ @Test
+ void testDeleteIndexDoesNotSuppressMissingTable() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier tid =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_missing_table"));
+
+ NoSuchTableException exception =
+ assertThrows(
+ NoSuchTableException.class,
+ () -> tc.alterTable(tid, TableChange.deleteIndex("idx_missing",
true)));
+ assertTrue(exception.getMessage().contains(tid.name()),
exception.getMessage());
+ }
+
@Test
void testCreateTableWithAutoIncrement() {
TableCatalog tc = catalog.asTableCatalog();
diff --git
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris4xIT.java
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris4xIT.java
index 6c5863dc98..e838a8274d 100644
---
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris4xIT.java
+++
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDoris4xIT.java
@@ -46,6 +46,7 @@ import org.apache.gravitino.Catalog;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.catalog.jdbc.config.JdbcConfig;
import org.apache.gravitino.client.GravitinoMetalake;
+import org.apache.gravitino.exceptions.NoSuchTableException;
import org.apache.gravitino.integration.test.container.ContainerSuite;
import org.apache.gravitino.integration.test.container.DorisContainer;
import org.apache.gravitino.integration.test.container.DorisImageName;
@@ -324,6 +325,137 @@ public class CatalogDoris4xIT extends BaseIT {
.untilAsserted(() -> assertEquals(0,
tc.loadTable(tid).index().length));
}
+ @Test
+ void testDeleteMissingIndexIfExists() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier tid =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_missing_idx"));
+
+ tc.createTable(
+ tid,
+ basicColumns(),
+ tableComment,
+ Collections.emptyMap(),
+ Transforms.EMPTY_TRANSFORM,
+ hashDist(),
+ null,
+ null);
+
+ tc.alterTable(tid, TableChange.deleteIndex("idx_missing", true));
+ assertEquals(0, tc.loadTable(tid).index().length);
+
+ IllegalArgumentException exception =
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> tc.alterTable(tid, TableChange.deleteIndex("idx_missing",
false)));
+ assertTrue(exception.getMessage().contains("Index does not exist"),
exception.getMessage());
+ }
+
+ @Test
+ void testDeleteExistingInvertedIndexWithIfExistsFalse() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier tid =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_existing_idx_strict"));
+
+ tc.createTable(
+ tid,
+ basicColumns(),
+ tableComment,
+ Collections.emptyMap(),
+ Transforms.EMPTY_TRANSFORM,
+ hashDist(),
+ null,
+ null);
+ tc.alterTable(
+ tid,
+ TableChange.addIndex(Index.IndexType.INVERTED, "idx_strict", new
String[][] {{colName2}}));
+
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(1,
tc.loadTable(tid).index().length));
+
+ tc.alterTable(tid, TableChange.deleteIndex("idx_strict", false));
+
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(0,
tc.loadTable(tid).index().length));
+ }
+
+ @Test
+ void testNoOpDeleteComposesWithUnrelatedChanges() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier addColumnFirst =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_column_first"));
+ NameIdentifier addColumnLast =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_column_last"));
+ NameIdentifier addIndexFirst =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_index_first"));
+ NameIdentifier addIndexLast =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_noop_index_last"));
+
+ for (NameIdentifier tid :
+ new NameIdentifier[] {addColumnFirst, addColumnLast, addIndexFirst,
addIndexLast}) {
+ tc.createTable(
+ tid,
+ basicColumns(),
+ tableComment,
+ Collections.emptyMap(),
+ Transforms.EMPTY_TRANSFORM,
+ hashDist(),
+ null,
+ null);
+ }
+
+ tc.alterTable(
+ addColumnFirst,
+ TableChange.deleteIndex("idx_missing", true),
+ TableChange.addColumn(new String[] {"col_extra"},
Types.VarCharType.of(100)));
+ tc.alterTable(
+ addColumnLast,
+ TableChange.addColumn(new String[] {"col_extra"},
Types.VarCharType.of(100)),
+ TableChange.deleteIndex("idx_missing", true));
+
+ for (NameIdentifier tid : new NameIdentifier[] {addColumnFirst,
addColumnLast}) {
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(3,
tc.loadTable(tid).columns().length));
+ }
+
+ tc.alterTable(
+ addIndexFirst,
+ TableChange.deleteIndex("idx_missing", true),
+ TableChange.addIndex(
+ Index.IndexType.INVERTED, "idx_unrelated", new String[][]
{{colName2}}));
+ tc.alterTable(
+ addIndexLast,
+ TableChange.addIndex(
+ Index.IndexType.INVERTED, "idx_unrelated", new String[][]
{{colName2}}),
+ TableChange.deleteIndex("idx_missing", true));
+
+ for (NameIdentifier tid : new NameIdentifier[] {addIndexFirst,
addIndexLast}) {
+ Awaitility.await()
+ .atMost(MAX_WAIT_IN_SECONDS, TimeUnit.SECONDS)
+ .pollInterval(WAIT_INTERVAL_IN_SECONDS, TimeUnit.SECONDS)
+ .untilAsserted(() -> assertEquals(1,
tc.loadTable(tid).index().length));
+ }
+ }
+
+ @Test
+ void testDeleteIndexDoesNotSuppressMissingTable() {
+ TableCatalog tc = catalog.asTableCatalog();
+ NameIdentifier tid =
+ NameIdentifier.of(schemaName,
GravitinoITUtils.genRandomName("t_missing_table"));
+
+ NoSuchTableException exception =
+ assertThrows(
+ NoSuchTableException.class,
+ () -> tc.alterTable(tid, TableChange.deleteIndex("idx_missing",
true)));
+ assertTrue(exception.getMessage().contains(tid.name()),
exception.getMessage());
+ }
+
@Test
void testCreateTableWithAutoIncrement() {
TableCatalog tc = catalog.asTableCatalog();
diff --git
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperations.java
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperations.java
index a503eb3c5e..93fc33d12f 100644
---
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperations.java
+++
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperations.java
@@ -669,20 +669,18 @@ public class TestDorisTableOperations extends TestDoris {
indexes);
JdbcTable load = TABLE_OPERATIONS.load(databaseName, tableName);
- // If ifExists is set to true then the code should not throw an exception
if the index doesn't
- // exist.
+ // If ifExists is set to true then the missing index should be a
metadata-aware no-op.
TableChange.DeleteIndex deleteIndex = new TableChange.DeleteIndex("uk_1",
true);
- String sql = DorisTableOperations.deleteIndexDefinition(null, deleteIndex);
- Assertions.assertEquals("DROP INDEX `uk_1`", sql);
+ String sql = DorisTableOperations.deleteIndexDefinition(load, deleteIndex);
+ Assertions.assertEquals("", sql);
- // The index existence check should only verify existence when ifExists is
false, preventing
- // failures when dropping non-existent indexes.
+ // Strict mode should continue to fail when dropping a non-existent index.
TableChange.DeleteIndex deleteIndex2 = new TableChange.DeleteIndex("uk_1",
false);
IllegalArgumentException thrown =
Assertions.assertThrows(
IllegalArgumentException.class,
() -> DorisTableOperations.deleteIndexDefinition(load,
deleteIndex2));
- Assertions.assertEquals("Index does not exist", thrown.getMessage());
+ Assertions.assertEquals("Index does not exist: uk_1", thrown.getMessage());
TableChange.DeleteIndex deleteIndex3 = new TableChange.DeleteIndex("uk_2",
false);
sql = DorisTableOperations.deleteIndexDefinition(load, deleteIndex3);
diff --git
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperationsSqlGeneration.java
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperationsSqlGeneration.java
index 105549ff7c..9b9b7de262 100644
---
a/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperationsSqlGeneration.java
+++
b/catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/operation/TestDorisTableOperationsSqlGeneration.java
@@ -54,6 +54,9 @@ import org.mockito.Mockito;
public class TestDorisTableOperationsSqlGeneration {
private static class TestableDorisTableOperations extends
DorisTableOperations {
+ private JdbcTable tableForAlter =
+
JdbcTable.builder().withName("test_table").withIndexes(Indexes.EMPTY_INDEXES).build();
+
public TestableDorisTableOperations() {
super.exceptionMapper = new JdbcExceptionConverter();
super.typeConverter = new DorisTypeConverter();
@@ -105,10 +108,14 @@ public class TestDorisTableOperationsSqlGeneration {
return generateAlterTableSql("database", tableName, changes);
}
+ void setTableForAlter(JdbcTable table) {
+ this.tableForAlter = table;
+ }
+
@Override
protected JdbcTable getOrCreateTable(
String databaseName, String tableName, JdbcTable lazyLoadCreateTable) {
- return JdbcTable.builder().withName(tableName).build();
+ return tableForAlter;
}
public String createTableSqlWithIndexes(
@@ -777,6 +784,114 @@ public class TestDorisTableOperationsSqlGeneration {
Assertions.assertEquals("DROP INDEX `idx_name`", sql);
}
+ @Test
+ public void testDeleteIndexDefinitionReturnsEmptyFragmentForMissingIndex() {
+ JdbcTable table = tableWithIndexes("idx_existing");
+
+ TableChange.DeleteIndex deleteIndex =
+ (TableChange.DeleteIndex) TableChange.deleteIndex("idx_missing", true);
+ Assertions.assertEquals("",
DorisTableOperations.deleteIndexDefinition(table, deleteIndex));
+
+ TableChange.DeleteIndex strictDeleteIndex =
+ (TableChange.DeleteIndex) TableChange.deleteIndex("idx_missing",
false);
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> DorisTableOperations.deleteIndexDefinition(table,
strictDeleteIndex));
+ Assertions.assertEquals("Index does not exist: idx_missing",
exception.getMessage());
+ }
+
+ @Test
+ public void testNoOpDeleteIsFilteredFromAlterSql() {
+ TestableDorisTableOperations ops = new TestableDorisTableOperations();
+ ops.setTableForAlter(tableWithIndexes());
+
+ Assertions.assertEquals(
+ "", ops.alterTableSql("test_table",
TableChange.deleteIndex("idx_missing", true)));
+
+ TableChange[] unrelatedChanges =
+ new TableChange[] {
+ TableChange.addColumn(new String[] {"col2"},
Types.IntegerType.get()),
+ TableChange.updateColumnComment(new String[] {"col1"}, "updated
comment"),
+ TableChange.setProperty(REPLICATION_FACTOR, "1"),
+ TableChange.addIndex(Index.IndexType.INVERTED, "idx_new", new
String[][] {{"col1"}})
+ };
+ String[] unrelatedFragments =
+ new String[] {
+ "ADD COLUMN `col2`",
+ "MODIFY COLUMN `col1` COMMENT 'updated comment'",
+ "set (",
+ "ADD INDEX `idx_new`"
+ };
+ for (int i = 0; i < unrelatedChanges.length; i++) {
+ TableChange unrelatedChange = unrelatedChanges[i];
+ String noOpFirstSql =
+ ops.alterTableSql(
+ "test_table", TableChange.deleteIndex("idx_missing", true),
unrelatedChange);
+ String noOpLastSql =
+ ops.alterTableSql(
+ "test_table", unrelatedChange,
TableChange.deleteIndex("idx_missing", true));
+
+ Assertions.assertFalse(noOpFirstSql.contains("DROP INDEX
`idx_missing`"), noOpFirstSql);
+ Assertions.assertFalse(noOpLastSql.contains("DROP INDEX `idx_missing`"),
noOpLastSql);
+ Assertions.assertTrue(noOpFirstSql.contains(unrelatedFragments[i]),
noOpFirstSql);
+ Assertions.assertTrue(noOpLastSql.contains(unrelatedFragments[i]),
noOpLastSql);
+ }
+ }
+
+ @Test
+ public void testIndexChangeConflictsFailFastBeforeJdbc() throws Exception {
+ TestableDorisTableOperations ops = new TestableDorisTableOperations();
+ DataSource dataSource = Mockito.mock(DataSource.class);
+ Connection connection = Mockito.mock(Connection.class);
+ Mockito.when(dataSource.getConnection()).thenReturn(connection);
+ ops.setDataSource(dataSource);
+
+ TableChange[][] duplicateDeleteChanges =
+ new TableChange[][] {
+ {TableChange.deleteIndex("idx", true),
TableChange.deleteIndex("idx", true)},
+ {TableChange.deleteIndex("idx", true),
TableChange.deleteIndex("idx", false)},
+ {TableChange.deleteIndex("idx", false),
TableChange.deleteIndex("idx", true)},
+ {TableChange.deleteIndex("idx", false),
TableChange.deleteIndex("idx", false)}
+ };
+ ops.setTableForAlter(tableWithIndexes("idx"));
+ for (TableChange[] changes : duplicateDeleteChanges) {
+ assertIndexChangeConflict(
+ ops,
+ connection,
+ "Index 'idx' cannot be deleted more than once in the same request",
+ changes);
+ }
+
+ TableChange.AddIndex addIndex =
+ (TableChange.AddIndex)
+ TableChange.addIndex(Index.IndexType.INVERTED, "idx", new
String[][] {{"col1"}});
+ TableChange[][] addDeleteChanges =
+ new TableChange[][] {
+ {addIndex, TableChange.deleteIndex("idx", true)},
+ {TableChange.deleteIndex("idx", true), addIndex},
+ {addIndex, TableChange.deleteIndex("idx", false)},
+ {TableChange.deleteIndex("idx", false), addIndex}
+ };
+ JdbcTable[] addDeleteTables =
+ new JdbcTable[] {
+ tableWithIndexes(), tableWithIndexes(), tableWithIndexes("idx"),
tableWithIndexes("idx")
+ };
+ for (int i = 0; i < addDeleteChanges.length; i++) {
+ ops.setTableForAlter(addDeleteTables[i]);
+ assertIndexChangeConflict(
+ ops,
+ connection,
+ "Index 'idx' cannot be added and deleted in the same request",
+ addDeleteChanges[i]);
+ }
+
+ ops.setTableForAlter(tableWithIndexes());
+ ops.alterTable("database", "test_table",
TableChange.deleteIndex("idx_missing", true));
+ Mockito.verify(connection, Mockito.never()).createStatement();
+ Mockito.verify(connection,
Mockito.never()).prepareStatement(Mockito.anyString());
+ }
+
@Test
public void testIsVersionAtLeast() {
// Exact match
@@ -839,6 +954,30 @@ public class TestDorisTableOperationsSqlGeneration {
exception.getMessage());
}
+ private static void assertIndexChangeConflict(
+ TestableDorisTableOperations ops,
+ Connection connection,
+ String expectedMessage,
+ TableChange[] changes)
+ throws Exception {
+ IllegalArgumentException exception =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () -> ops.alterTable("database", "test_table", changes));
+
+ Assertions.assertEquals(expectedMessage, exception.getMessage());
+ Mockito.verify(connection, Mockito.never()).createStatement();
+ Mockito.verify(connection,
Mockito.never()).prepareStatement(Mockito.anyString());
+ }
+
+ private static JdbcTable tableWithIndexes(String... indexNames) {
+ Index[] indexes = new Index[indexNames.length];
+ for (int i = 0; i < indexNames.length; i++) {
+ indexes[i] = Indexes.of(Index.IndexType.INVERTED, indexNames[i], new
String[][] {{"col1"}});
+ }
+ return
JdbcTable.builder().withName("test_table").withIndexes(indexes).build();
+ }
+
private static void assertInvalidAddIndex(String[][] fields, String
expectedMessage) {
TableChange.AddIndex addIndex =
(TableChange.AddIndex) TableChange.addIndex(Index.IndexType.INVERTED,
"idx_name", fields);
