gemini-code-assist[bot] commented on code in PR #38952:
URL: https://github.com/apache/beam/pull/38952#discussion_r3406031007
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/parser/SqlDdlNodes.java:
##########
@@ -72,7 +72,7 @@ static Pair<CalciteSchema, String> schema(
return Pair.of(checkStateNotNull(schema, "Got null sub-schema for path
'%s'", path), name(id));
}
- private static @Nullable CalciteSchema childSchema(CalciteSchema rootSchema,
List<String> path) {
+ public static @Nullable CalciteSchema childSchema(CalciteSchema rootSchema,
List<String> path) {
@Nullable CalciteSchema schema = rootSchema;
for (String p : path) {
Review Comment:
Since `childSchema` is now a `public` method, it can be called by external
components with arbitrary arguments. To prevent a potential
`NullPointerException`, we should add a defensive null check for the `path`
parameter before iterating over it.
```suggestion
public static @Nullable CalciteSchema childSchema(CalciteSchema
rootSchema, List<String> path) {
if (path == null) {
return null;
}
@Nullable CalciteSchema schema = rootSchema;
for (String p : path) {
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/catalog/InMemoryCatalog.java:
##########
@@ -111,12 +110,19 @@ public Collection<String> databases() {
@Override
public boolean dropDatabase(String database, boolean cascade) {
- checkState(!cascade, "%s does not support CASCADE.",
getClass().getSimpleName());
+ if (!databases.contains(database)) {
+ return false;
+ }
+ 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:
We can optimize this method by avoiding the redundant
`databases.contains(database)` check. By directly calling
`databases.remove(database)` and checking its return value, we can reduce the
number of lookups on the collection and simplify the control flow.
```java
@Override
public boolean dropDatabase(String database, boolean cascade) {
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 (!removed) {
return false;
}
if (database.equals(currentDatabase)) {
currentDatabase = null;
}
metaStores.remove(database);
return removed;
}
```
--
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]
