From 631b579bbd31bcb4a8285045ee60db8c0f6e1419 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 21 Dec 2023 19:39:04 +0800 Subject: [PATCH] [#1112] fix(jdbc): deleteColumn throws exception for table with ifExisting = true. (#1230) ### What changes were proposed in this pull request? When 'ifExisting' is set to true in the 'deleteColumn' function, it will validate the existence of the specified column. If the column does not exist, it will throw an exception. ### Why are the changes needed? Fix: #1112 ### Does this PR introduce _any_ user-facing change? Users should be aware that when using 'deleteColumn,' it may throw a new exception, 'IllegalArgumentException.' ### How was this patch tested? UT Co-authored-by: Clearvive <143773256+Clearvive@users.noreply.github.com> Co-authored-by: Clearvive --- .../jdbc/operation/JdbcTableOperations.java | 9 +++++-- .../mysql/operation/MysqlTableOperations.java | 24 +++++++++++++++-- .../operation/PostgreSqlTableOperations.java | 26 ++++++++++++++++--- .../jdbc/mysql/TestMysqlTableOperations.java | 21 +++++++++------ .../TestPostgreSqlTableOperations.java | 20 ++++++++------ 5 files changed, 77 insertions(+), 23 deletions(-) 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 ce471880a9e..50250d7659a 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 @@ -32,6 +32,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. */ @@ -350,8 +351,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( @@ -362,6 +367,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); @@ -397,11 +407,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 afa96964797..9dac0cc27ff 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));