gemini-code-assist[bot] commented on code in PR #38952:
URL: https://github.com/apache/beam/pull/38952#discussion_r3405936604
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/InMemoryCatalog.java:
##########
@@ -111,12 +110,16 @@ public Collection<String> databases() {
@Override
public boolean dropDatabase(String database, boolean cascade) {
- checkState(!cascade, "%s does not support CASCADE.",
getClass().getSimpleName());
+ MetaStore metaStore = metaStores.get(database);
+ if (!cascade && metaStore != null && !metaStore.getTables().isEmpty()) {
+ throw new IllegalStateException("Database '" + database + "' is not
empty.");
+ }
boolean removed = databases.remove(database);
if (database.equals(currentDatabase)) {
currentDatabase = null;
}
+ metaStores.remove(database);
return removed;
Review Comment:
It is safer and cleaner to only clean up `currentDatabase` and `metaStores`
if the database was actually successfully removed from the `databases` set.
This prevents inconsistent state and unnecessary map operations if the database
did not exist.
```java
boolean removed = databases.remove(database);
if (removed) {
if (database.equals(currentDatabase)) {
currentDatabase = null;
}
metaStores.remove(database);
}
return removed;
```
##########
sdks/java/extensions/sql/src/test/java/org/apache/beam/sdk/extensions/sql/BeamSqlCliDatabaseTest.java:
##########
@@ -126,6 +135,33 @@ public void testDropDatabase_nonexistent() {
cli.execute("DROP DATABASE my_database");
}
+ @Test
+ public void testDropDatabase_notEmpty_restrict() {
+ cli.execute("CREATE DATABASE db_1");
+ cli.execute("USE db_1");
+
+ TestTableProvider testTableProvider = new TestTableProvider();
+ catalogManager.registerTableProvider(testTableProvider);
+ cli.execute("CREATE EXTERNAL TABLE person(id int, name varchar, age int)
TYPE 'test'");
+
+ thrown.expect(RuntimeException.class);
+ thrown.expectMessage("Database 'db_1' is not empty.");
Review Comment:
Expecting the specific `IllegalStateException` class makes the test more
precise and robust than expecting the generic `RuntimeException`, which could
mask other unexpected runtime exceptions (such as `NullPointerException`).
```suggestion
thrown.expect(IllegalStateException.class);
thrown.expectMessage("Database 'db_1' is not empty.");
```
--
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]
