Copilot commented on code in PR #9502:
URL: https://github.com/apache/gravitino/pull/9502#discussion_r2646643688
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -835,7 +837,9 @@ private boolean containsUserCreatedSchemas(
catalogWrapper.doWithSchemaOps(
schemaOps ->
schemaOps.listSchemas(
- Namespace.of(catalogEntity.namespace().level(0),
catalogEntity.name())));
+ NamespaceUtil.ofSchema(
+ catalogWrapper.catalog.entity().namespace().level(0),
+ catalogWrapper.catalog.name())));
Review Comment:
Direct field access should be avoided in favor of the public accessor
method. Consider using `catalogWrapper.catalog().entity()` and
`catalogWrapper.catalog().name()` instead of directly accessing the catalog
field for better encapsulation and consistency with the rest of the codebase.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -738,28 +739,24 @@ public boolean dropCatalog(NameIdentifier ident, boolean
force)
metalakeIdent,
LockType.WRITE,
() -> {
- checkMetalake(metalakeIdent, store);
try {
boolean catalogInUse = catalogInUse(store, ident);
if (catalogInUse && !force) {
throw new CatalogInUseException(
"Catalog %s is in use, please disable it first or use force
option", ident);
}
- Namespace schemaNamespace =
Namespace.of(ident.namespace().level(0), ident.name());
- CatalogWrapper catalogWrapper = loadCatalogAndWrap(ident);
-
+ Namespace schemaNs = Namespace.of(ident.namespace().level(0),
ident.name());
List<SchemaEntity> schemaEntities =
- store.list(schemaNamespace, SchemaEntity.class,
EntityType.SCHEMA);
- CatalogEntity catalogEntity = store.get(ident, EntityType.CATALOG,
CatalogEntity.class);
+ store.list(schemaNs, SchemaEntity.class, EntityType.SCHEMA);
- if (!force
- && containsUserCreatedSchemas(schemaEntities, catalogEntity,
catalogWrapper)) {
+ CatalogWrapper catalogWrapper = loadCatalogAndWrap(ident);
+ if (!force && containsUserCreatedSchemas(schemaEntities,
catalogWrapper)) {
throw new NonEmptyCatalogException(
"Catalog %s has schemas, please drop them first or use force
option", ident);
}
- if (includeManagedEntities(catalogEntity)) {
+ if (isManagedSchema(catalogWrapper)) {
// code reach here in two cases:
Review Comment:
Grammar issue in comment: "code reach here" should be "code reaches here".
```suggestion
// code reaches here in two cases:
```
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -893,6 +893,15 @@ private boolean getCatalogInUseValue(EntityStore store,
NameIdentifier catalogId
}
}
+ private boolean isManagedSchema(CatalogWrapper catalogWrapper) {
+ try {
+ return
catalogWrapper.capabilities().managedStorage(Capability.Scope.SCHEMA).supported();
+ } catch (Exception e) {
+ LOG.warn("Failed to get capabilities for catalog, assuming not managed",
e);
+ return false;
+ }
+ }
Review Comment:
The new private method isManagedSchema lacks documentation. Add a Javadoc
comment explaining that this method checks whether the catalog supports managed
storage at the SCHEMA scope, which determines if physical data should be
deleted when dropping schemas. This is important for understanding when managed
entities (like Lance tables) will have their physical data cleaned up versus
just their metadata.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -738,28 +739,24 @@ public boolean dropCatalog(NameIdentifier ident, boolean
force)
metalakeIdent,
LockType.WRITE,
() -> {
- checkMetalake(metalakeIdent, store);
try {
boolean catalogInUse = catalogInUse(store, ident);
if (catalogInUse && !force) {
throw new CatalogInUseException(
"Catalog %s is in use, please disable it first or use force
option", ident);
}
- Namespace schemaNamespace =
Namespace.of(ident.namespace().level(0), ident.name());
- CatalogWrapper catalogWrapper = loadCatalogAndWrap(ident);
-
+ Namespace schemaNs = Namespace.of(ident.namespace().level(0),
ident.name());
List<SchemaEntity> schemaEntities =
- store.list(schemaNamespace, SchemaEntity.class,
EntityType.SCHEMA);
- CatalogEntity catalogEntity = store.get(ident, EntityType.CATALOG,
CatalogEntity.class);
+ store.list(schemaNs, SchemaEntity.class, EntityType.SCHEMA);
Review Comment:
Variable name 'schemaNs' is less clear than the previous 'schemaNamespace'.
While brevity can be good, 'schemaNs' is not immediately obvious as an
abbreviation for 'schema namespace'. Consider using a more descriptive name
like 'schemaNamespace' or 'catalogSchemaNamespace' for better code readability.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -805,28 +803,32 @@ && containsUserCreatedSchemas(schemaEntities,
catalogEntity, catalogWrapper)) {
* </ul>
*
* @param schemaEntities The list of schema entities to check.
- * @param catalogEntity The catalog entity to which the schemas belong.
* @param catalogWrapper The catalog wrapper for the catalog.
* @return True if the list of schema entities contains any valid
user-created schemas, false
* otherwise.
* @throws Exception If an error occurs while checking the schemas.
*/
private boolean containsUserCreatedSchemas(
- List<SchemaEntity> schemaEntities, CatalogEntity catalogEntity,
CatalogWrapper catalogWrapper)
- throws Exception {
+ List<SchemaEntity> schemaEntities, CatalogWrapper catalogWrapper) throws
Exception {
if (schemaEntities.isEmpty()) {
return false;
}
+ if (isManagedSchema(catalogWrapper)) {
+ // For managed schemas, if there are any schema entities, they are
considered user-created,
+ // no need to check further.
+ return !schemaEntities.isEmpty();
+ }
+
if (schemaEntities.size() == 1) {
- if ("kafka".equals(catalogEntity.getProvider())) {
+ String provider = catalogWrapper.catalog.provider();
Review Comment:
Direct field access should be avoided in favor of the public accessor
method. Consider using `catalogWrapper.catalog().provider()` instead of
`catalogWrapper.catalog.provider()` for better encapsulation and consistency
with the rest of the codebase. The CatalogWrapper class provides a public
catalog() method specifically for this purpose.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -738,28 +739,24 @@ public boolean dropCatalog(NameIdentifier ident, boolean
force)
metalakeIdent,
LockType.WRITE,
() -> {
- checkMetalake(metalakeIdent, store);
try {
boolean catalogInUse = catalogInUse(store, ident);
if (catalogInUse && !force) {
throw new CatalogInUseException(
"Catalog %s is in use, please disable it first or use force
option", ident);
}
Review Comment:
The removal of the checkMetalake call changes the exception behavior.
Previously, if the metalake was not in use, a MetalakeNotInUseException would
be thrown. Now, if the metalake is not in use, the catalogInUse method returns
false, which results in a CatalogInUseException being thrown instead (when
force=false). This changes the user-facing API behavior and may break existing
error handling logic in clients. Consider either restoring the checkMetalake
call or documenting this as an intentional API change.
##########
core/src/main/java/org/apache/gravitino/catalog/CatalogManager.java:
##########
@@ -805,28 +803,32 @@ && containsUserCreatedSchemas(schemaEntities,
catalogEntity, catalogWrapper)) {
* </ul>
*
* @param schemaEntities The list of schema entities to check.
- * @param catalogEntity The catalog entity to which the schemas belong.
* @param catalogWrapper The catalog wrapper for the catalog.
* @return True if the list of schema entities contains any valid
user-created schemas, false
* otherwise.
* @throws Exception If an error occurs while checking the schemas.
*/
private boolean containsUserCreatedSchemas(
- List<SchemaEntity> schemaEntities, CatalogEntity catalogEntity,
CatalogWrapper catalogWrapper)
- throws Exception {
+ List<SchemaEntity> schemaEntities, CatalogWrapper catalogWrapper) throws
Exception {
if (schemaEntities.isEmpty()) {
return false;
}
+ if (isManagedSchema(catalogWrapper)) {
+ // For managed schemas, if there are any schema entities, they are
considered user-created,
+ // no need to check further.
+ return !schemaEntities.isEmpty();
Review Comment:
The return statement `return !schemaEntities.isEmpty()` is redundant and
potentially confusing. At this point in the code, we already know that
schemaEntities is not empty because of the check at line 813. This condition
will always evaluate to true here. Consider simplifying this to `return true`
with an updated comment explaining that for managed schemas, any schema
entities present are considered user-created.
```suggestion
// For managed schemas, any existing schema entities are considered
user-created. At this
// point we already know schemaEntities is not empty, so we can return
true directly
// without further checks.
return 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]
