diff --git a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java index f8f78161166..1fcb53fb8a3 100644 --- a/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java +++ b/catalogs/catalog-jdbc-common/src/main/java/com/datastrato/gravitino/catalog/jdbc/operation/JdbcTableOperations.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Map; import javax.sql.DataSource; +import org.apache.commons.lang3.StringUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -157,8 +158,12 @@ public void alterTable(String databaseName, String tableName, TableChange... cha throws NoSuchTableException { LOG.info("Attempting to alter table {} from database {}", tableName, databaseName); try (Connection connection = getConnection(databaseName)) { - JdbcConnectorUtils.executeUpdate( - connection, generateAlterTableSql(databaseName, tableName, changes)); + String sql = generateAlterTableSql(databaseName, tableName, changes); + if (StringUtils.isEmpty(sql)) { + LOG.info("No changes to alter table {} from database {}", tableName, databaseName); + return; + } + JdbcConnectorUtils.executeUpdate(connection, sql); LOG.info("Alter table {} from database {}", tableName, databaseName); } catch (final SQLException se) { throw this.exceptionMapper.toGravitinoException(se); diff --git a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java index 48a6f91307c..1b19b2cb788 100644 --- a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java +++ b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/operation/MysqlTableOperations.java @@ -37,6 +37,7 @@ import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.collections4.MapUtils; import org.apache.commons.lang3.ArrayUtils; +import org.apache.commons.lang3.BooleanUtils; import org.apache.commons.lang3.StringUtils; /** Table operations for MySQL. */ @@ -322,7 +323,11 @@ protected String generateAlterTableSql( updateColumnPositionFieldDefinition(updateColumnPosition, lazyLoadCreateTable)); } else if (change instanceof TableChange.DeleteColumn) { TableChange.DeleteColumn deleteColumn = (TableChange.DeleteColumn) change; - alterSql.add(deleteColumnFieldDefinition(deleteColumn)); + lazyLoadCreateTable = getOrCreateTable(databaseName, tableName, lazyLoadCreateTable); + String deleteColSql = deleteColumnFieldDefinition(deleteColumn, lazyLoadCreateTable); + if (StringUtils.isNotEmpty(deleteColSql)) { + alterSql.add(deleteColSql); + } } else if (change instanceof TableChange.UpdateColumnNullability) { lazyLoadCreateTable = getOrCreateTable(databaseName, tableName, lazyLoadCreateTable); alterSql.add( @@ -353,6 +358,9 @@ protected String generateAlterTableSql( alterSql.add("COMMENT '" + newComment + "'"); } + if (CollectionUtils.isEmpty(alterSql)) { + return ""; + } // Return the generated SQL statement String result = "ALTER TABLE " + tableName + "\n" + String.join(",\n", alterSql) + ";"; LOG.info("Generated alter table:{} sql: {}", databaseName + "." + tableName, result); @@ -496,11 +504,23 @@ private String updateColumnPositionFieldDefinition( return columnDefinition.toString(); } - private String deleteColumnFieldDefinition(TableChange.DeleteColumn deleteColumn) { + private String deleteColumnFieldDefinition( + TableChange.DeleteColumn deleteColumn, CreateTable lazyLoadCreateTable) { if (deleteColumn.fieldName().length > 1) { throw new UnsupportedOperationException("Mysql does not support nested column names."); } String col = deleteColumn.fieldName()[0]; + boolean colExists = + lazyLoadCreateTable.getColumnDefinitions().stream() + .map(MysqlTableOperations::getColumnName) + .anyMatch(s -> StringUtils.equals(col, s)); + if (!colExists) { + if (BooleanUtils.isTrue(deleteColumn.getIfExists())) { + return ""; + } else { + throw new IllegalArgumentException("Delete column does not exist: " + col); + } + } return "DROP COLUMN " + col; } diff --git a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java index a89b83308c1..cb7c47080f4 100644 --- a/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java +++ b/catalogs/catalog-jdbc-postgresql/src/main/java/com/datastrato/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java @@ -33,6 +33,7 @@ import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.collections4.MapUtils; import org.apache.commons.lang3.ArrayUtils; +import org.apache.commons.lang3.BooleanUtils; import org.apache.commons.lang3.StringUtils; /** Table operations for PostgreSQL. */ @@ -351,8 +352,12 @@ protected String generateAlterTableSql( } else if (change instanceof TableChange.UpdateColumnPosition) { throw new IllegalArgumentException("PostgreSQL does not support column position."); } else if (change instanceof TableChange.DeleteColumn) { + lazyLoadTable = getOrCreateTable(schemaName, tableName, lazyLoadTable); TableChange.DeleteColumn deleteColumn = (TableChange.DeleteColumn) change; - alterSql.add(deleteColumnFieldDefinition(deleteColumn, tableName)); + String deleteColSql = deleteColumnFieldDefinition(deleteColumn, lazyLoadTable); + if (StringUtils.isNotEmpty(deleteColSql)) { + alterSql.add(deleteColSql); + } } else if (change instanceof TableChange.UpdateColumnNullability) { alterSql.add( updateColumnNullabilityDefinition( @@ -363,6 +368,11 @@ protected String generateAlterTableSql( } } + // If there is no change, return directly + if (alterSql.isEmpty()) { + return ""; + } + // Return the generated SQL statement String result = String.join("\n", alterSql); LOG.info("Generated alter table:{}.{} sql: {}", schemaName, tableName, result); @@ -398,11 +408,21 @@ private String updateCommentDefinition( } private String deleteColumnFieldDefinition( - TableChange.DeleteColumn deleteColumn, String tableName) { + TableChange.DeleteColumn deleteColumn, JdbcTable table) { if (deleteColumn.fieldName().length > 1) { throw new UnsupportedOperationException("PostgreSQL does not support nested column names."); } - return "ALTER TABLE " + tableName + " DROP COLUMN " + deleteColumn.fieldName()[0] + ";"; + String col = deleteColumn.fieldName()[0]; + boolean colExists = + Arrays.stream(table.columns()).anyMatch(s -> StringUtils.equals(col, s.name())); + if (!colExists) { + if (BooleanUtils.isTrue(deleteColumn.getIfExists())) { + return ""; + } else { + throw new IllegalArgumentException("Delete column does not exist: " + col); + } + } + return "ALTER TABLE " + table.name() + " DROP COLUMN " + deleteColumn.fieldName()[0] + ";"; } private String updateColumnTypeFieldDefinition( diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/TestMysqlTableOperations.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/TestMysqlTableOperations.java index 206a573b557..8015d1caa6e 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/TestMysqlTableOperations.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/TestMysqlTableOperations.java @@ -9,7 +9,6 @@ import com.datastrato.gravitino.catalog.jdbc.JdbcColumn; import com.datastrato.gravitino.catalog.jdbc.JdbcTable; -import com.datastrato.gravitino.exceptions.GravitinoRuntimeException; import com.datastrato.gravitino.exceptions.NoSuchTableException; import com.datastrato.gravitino.integration.test.util.GravitinoITUtils; import com.datastrato.gravitino.rel.TableChange; @@ -138,19 +137,25 @@ public void testOperationTable() { load = TABLE_OPERATIONS.load(TEST_DB_NAME, newName); assertionsTableInfo(newName, tableComment, columns, properties, load); - GravitinoRuntimeException gravitinoRuntimeException = + IllegalArgumentException illegalArgumentException = Assertions.assertThrows( - GravitinoRuntimeException.class, + IllegalArgumentException.class, () -> TABLE_OPERATIONS.alterTable( TEST_DB_NAME, newName, - TableChange.deleteColumn(new String[] {newColumn.name()}, true))); + TableChange.deleteColumn(new String[] {newColumn.name()}, false))); + Assertions.assertEquals( + "Delete column does not exist: " + newColumn.name(), illegalArgumentException.getMessage()); + Assertions.assertDoesNotThrow( + () -> + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, + newName, + TableChange.deleteColumn(new String[] {newColumn.name()}, true))); - Assertions.assertTrue( - gravitinoRuntimeException - .getMessage() - .contains("Can't DROP 'col_5'; check that column/key exists")); + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, newName, TableChange.deleteColumn(new String[] {newColumn.name()}, true)); Assertions.assertDoesNotThrow(() -> TABLE_OPERATIONS.purge(TEST_DB_NAME, newName)); Assertions.assertThrows( NoSuchTableException.class, () -> TABLE_OPERATIONS.purge(TEST_DB_NAME, newName)); diff --git a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/TestPostgreSqlTableOperations.java b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/TestPostgreSqlTableOperations.java index 3451837f75a..066044c75d5 100644 --- a/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/TestPostgreSqlTableOperations.java +++ b/integration-test/src/test/java/com/datastrato/gravitino/integration/test/catalog/jdbc/postgresql/TestPostgreSqlTableOperations.java @@ -12,7 +12,6 @@ import com.datastrato.gravitino.catalog.postgresql.converter.PostgreSqlTypeConverter; import com.datastrato.gravitino.catalog.postgresql.operation.PostgreSqlSchemaOperations; import com.datastrato.gravitino.catalog.postgresql.operation.PostgreSqlTableOperations; -import com.datastrato.gravitino.exceptions.GravitinoRuntimeException; import com.datastrato.gravitino.exceptions.NoSuchTableException; import com.datastrato.gravitino.integration.test.util.GravitinoITUtils; import com.datastrato.gravitino.rel.TableChange; @@ -180,23 +179,28 @@ public void testOperationTable() { // delete column TABLE_OPERATIONS.alterTable( TEST_DB_NAME, newName, TableChange.deleteColumn(new String[] {newColumn.name()}, true)); + load = TABLE_OPERATIONS.load(TEST_DB_NAME, newName); alterColumns.remove(newColumn); assertionsTableInfo(newName, tableComment, alterColumns, properties, load); - GravitinoRuntimeException gravitinoRuntimeException = + IllegalArgumentException illegalArgumentException = Assertions.assertThrows( - GravitinoRuntimeException.class, + IllegalArgumentException.class, () -> TABLE_OPERATIONS.alterTable( TEST_DB_NAME, newName, - TableChange.deleteColumn(new String[] {newColumn.name()}, true))); + TableChange.deleteColumn(new String[] {newColumn.name()}, false))); + Assertions.assertEquals( + "Delete column does not exist: " + newColumn.name(), illegalArgumentException.getMessage()); - Assertions.assertTrue( - gravitinoRuntimeException - .getMessage() - .contains("ERROR: column \"col_5\" of relation \"new_table\" does not exist")); + Assertions.assertDoesNotThrow( + () -> + TABLE_OPERATIONS.alterTable( + TEST_DB_NAME, + newName, + TableChange.deleteColumn(new String[] {newColumn.name()}, true))); Assertions.assertDoesNotThrow(() -> TABLE_OPERATIONS.purge(TEST_DB_NAME, newName)); Assertions.assertThrows( NoSuchTableException.class, () -> TABLE_OPERATIONS.purge(TEST_DB_NAME, newName));