[#1112] fix(jdbc): deleteColumn throws exception for table with ifExisting = true. #1200
Conversation
...a/com/datastrato/gravitino/integration/test/catalog/jdbc/mysql/TestMysqlTableOperations.java
Show resolved
Hide resolved
| .anyMatch(s -> StringUtils.equals(col, s)); | ||
| if (!colExists) { | ||
| if (BooleanUtils.isTrue(deleteColumn.getIfExists())) { | ||
| return ""; |
There was a problem hiding this comment.
Does MySQL support ALTER TABLE table_name syntax if we get a empty string here directly?
There was a problem hiding this comment.
MySQL supports such statements
There was a problem hiding this comment.
suggest to add a check to skip execute if Alter SQL content is empty.
There was a problem hiding this comment.
In that case, there will be more code modifications, and all upper layers need to make a judgment once
There was a problem hiding this comment.
I have modified this part of the code. Could you please help me review it
| @@ -322,7 +323,8 @@ 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); | |||
| alterSql.add(deleteColumnFieldDefinition(deleteColumn, lazyLoadCreateTable)); | |||
There was a problem hiding this comment.
I think you can judge whether the return value of deleteColumnFieldDefinition is empty, if it's not empty, we can then put it into alterSql;
Line 358 would not work if we do multiple changes at the same time.
| if (BooleanUtils.isTrue(deleteColumn.getIfExists())) { | ||
| return ""; | ||
| } else { | ||
| throw new IllegalArgumentException("delete column not exists: " + col); |
There was a problem hiding this comment.
"Delete column does not exist: "
| if (BooleanUtils.isTrue(deleteColumn.getIfExists())) { | ||
| return ""; | ||
| } else { | ||
| throw new IllegalArgumentException("delete column not exists: " + col); |
There was a problem hiding this comment.
"Delete column does not exist: ".
…sting = true. (#1200) ### 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 <clearvive@datastrato.com>
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