Copilot commented on code in PR #9318:
URL: https://github.com/apache/gravitino/pull/9318#discussion_r2580081631
##########
catalogs/catalog-jdbc-starrocks/src/test/java/org/apache/gravitino/catalog/starrocks/operation/TestStarRocksTableOperations.java:
##########
@@ -605,9 +604,15 @@ public void testUnsupportedPropertyThrows() {
null,
indexes);
- TableChange.SetProperty change = new
TableChange.SetProperty("unsupported.property", "value");
+ Assertions.assertDoesNotThrow(
+ () ->
+ TABLE_OPERATIONS.alterTable(
+ databaseName, tableName,
TableChange.setProperty("replication_num", "2")));
+
Assertions.assertThrows(
- IllegalPropertyException.class,
- () -> TABLE_OPERATIONS.alterTable(databaseName, tableName, change));
+ IllegalArgumentException.class,
+ () ->
+ TABLE_OPERATIONS.alterTable(
+ databaseName, tableName,
TableChange.setProperty("unsupported.property", "VIEW")));
Review Comment:
The test validates that unsupported properties are rejected and that
`replication_num` (from SUPPORTED_MODIFY_PROPERTIES) works correctly. However,
it doesn't test properties with the `dynamic_partition.*` or `binlog.*`
prefixes, which are also allowed by the fixed logic. Consider adding test cases
for these scenarios:
```java
// Test dynamic_partition prefix
Assertions.assertDoesNotThrow(
() ->
TABLE_OPERATIONS.alterTable(
databaseName, tableName,
TableChange.setProperty("dynamic_partition.enable", "true")));
// Test binlog prefix
Assertions.assertDoesNotThrow(
() ->
TABLE_OPERATIONS.alterTable(
databaseName, tableName,
TableChange.setProperty("binlog.enable", "true")));
```
--
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]
