Copilot commented on code in PR #9502:
URL: https://github.com/apache/gravitino/pull/9502#discussion_r2646948302
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/generic/GenericCatalogCapability.java:
##########
@@ -27,7 +27,8 @@ public class GenericCatalogCapability implements Capability {
@Override
public CapabilityResult managedStorage(Scope scope) {
if (Objects.requireNonNull(scope) == Scope.TABLE
- || Objects.requireNonNull(scope) == Scope.SCHEMA) {
+ || Objects.requireNonNull(scope) == Scope.SCHEMA
+ || Objects.requireNonNull(scope) == Scope.CATALOG) {
Review Comment:
The scope parameter is checked with Objects.requireNonNull twice in the same
condition. Consider calling it once and storing the result, or removing the
redundant second call. For example: if (scope == Scope.TABLE || scope ==
Scope.SCHEMA || scope == Scope.CATALOG) after a single
Objects.requireNonNull(scope) check at the beginning of the method.
##########
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java:
##########
@@ -934,4 +936,115 @@ void testRegisterWithNonExistLocation() {
boolean dropSuccess =
catalog.asTableCatalog().dropTable(newTableIdentifier);
Assertions.assertTrue(dropSuccess);
}
+
+ @Test
+ void testDropCatalogWithManagedAndExternalEntities() {
+ // Create a new catalog for this test to avoid interfering with other tests
+ String testCatalogName =
GravitinoITUtils.genRandomName("drop_catalog_test");
+ Map<String, String> catalogProperties = Maps.newHashMap();
+ metalake.createCatalog(
+ testCatalogName,
+ Catalog.Type.RELATIONAL,
+ provider,
+ "Test catalog for drop",
+ catalogProperties);
+
+ Catalog testCatalog = metalake.loadCatalog(testCatalogName);
+
+ // Create a schema
+ String testSchemaName = GravitinoITUtils.genRandomName(SCHEMA_PREFIX);
+ Map<String, String> schemaProperties = createSchemaProperties();
+ testCatalog.asSchemas().createSchema(testSchemaName, "Test schema",
schemaProperties);
+
+ Column[] columns = createColumns();
+
+ // Create a managed (non-external) Lance table
+ String managedTableName = GravitinoITUtils.genRandomName(TABLE_PREFIX +
"_managed");
+ NameIdentifier managedTableIdentifier = NameIdentifier.of(testSchemaName,
managedTableName);
+ String managedTableLocation =
+ String.format("%s/%s/%s", tempDirectory, testSchemaName,
managedTableName);
+
+ Map<String, String> managedTableProperties = createProperties();
+ managedTableProperties.put(Table.PROPERTY_TABLE_FORMAT, "lance");
+ managedTableProperties.put(Table.PROPERTY_LOCATION, managedTableLocation);
+
+ Table managedTable =
+ testCatalog
+ .asTableCatalog()
+ .createTable(
+ managedTableIdentifier,
+ columns,
+ "Managed table",
+ managedTableProperties,
+ Transforms.EMPTY_TRANSFORM,
+ null,
+ null);
+
+ Assertions.assertNotNull(managedTable);
+ File managedTableDir = new File(managedTableLocation);
+ Assertions.assertTrue(
+ managedTableDir.exists(), "Managed table directory should exist after
creation");
+
+ // Create an external Lance table
+ String externalTableName = GravitinoITUtils.genRandomName(TABLE_PREFIX +
"_external");
+ NameIdentifier externalTableIdentifier = NameIdentifier.of(testSchemaName,
externalTableName);
+ String externalTableLocation =
+ String.format("%s/%s/%s", tempDirectory, testSchemaName,
externalTableName);
+
+ Map<String, String> externalTableProperties = createProperties();
+ externalTableProperties.put(Table.PROPERTY_TABLE_FORMAT, "lance");
+ externalTableProperties.put(Table.PROPERTY_LOCATION,
externalTableLocation);
+ externalTableProperties.put(Table.PROPERTY_EXTERNAL, "true");
+
+ Table externalTable =
+ testCatalog
+ .asTableCatalog()
+ .createTable(
+ externalTableIdentifier,
+ columns,
+ "External table",
+ externalTableProperties,
+ Transforms.EMPTY_TRANSFORM,
+ null,
+ null);
+
+ Assertions.assertNotNull(externalTable);
+ File externalTableDir = new File(externalTableLocation);
+ Assertions.assertTrue(
+ externalTableDir.exists(), "External table directory should exist
after creation");
+
+ // Verify both tables exist in catalog
+ Table loadedManagedTable =
testCatalog.asTableCatalog().loadTable(managedTableIdentifier);
+ Assertions.assertNotNull(loadedManagedTable);
+
+ Table loadedExternalTable =
testCatalog.asTableCatalog().loadTable(externalTableIdentifier);
+ Assertions.assertNotNull(loadedExternalTable);
+
+ // Drop the catalog with force=true
+ boolean catalogDropped = metalake.dropCatalog(testCatalogName, true);
+ Assertions.assertTrue(catalogDropped, "Catalog should be dropped
successfully");
+
+ // Verify the catalog is dropped
+ Assertions.assertThrows(
+ NoSuchCatalogException.class,
+ () -> metalake.loadCatalog(testCatalogName),
+ "Catalog should not exist after drop");
+
+ // Verify the managed table's physical directory is removed
+ Assertions.assertFalse(
+ managedTableDir.exists(),
+ "Managed table directory should be removed after dropping catalog");
+
+ // Verify the external table's physical directory is preserved
+ Assertions.assertTrue(
+ externalTableDir.exists(),
+ "External table directory should be preserved after dropping catalog");
+
+ // Clean up external table directory manually
+ try {
+ FileUtils.deleteDirectory(externalTableDir);
+ } catch (IOException e) {
+ LOG.warn("Failed to delete external table directory: {}",
externalTableLocation, e);
+ }
Review Comment:
The test creates tables in subdirectories under tempDirectory (e.g.,
tempDirectory/testSchemaName/managedTableName) but only manually cleans up the
external table directory. Consider also cleaning up the schema directory
(tempDirectory/testSchemaName) to prevent accumulation of empty directories
from test runs, especially if the managed table deletion doesn't remove the
parent schema directory.
```suggestion
}
// Clean up the schema directory (parent of the managed table directory)
if it is empty
File schemaDir = managedTableDir.getParentFile();
if (schemaDir != null && schemaDir.exists() && schemaDir.isDirectory()) {
String[] children = schemaDir.list();
if (children != null && children.length == 0) {
if (!schemaDir.delete()) {
LOG.warn("Failed to delete schema directory: {}",
schemaDir.getAbsolutePath());
}
}
}
```
##########
catalogs/catalog-fileset/src/main/java/org/apache/gravitino/catalog/fileset/FilesetCatalogCapability.java:
##########
@@ -25,7 +25,8 @@
public class FilesetCatalogCapability implements Capability {
@Override
public CapabilityResult managedStorage(Scope scope) {
- if (Objects.requireNonNull(scope) == Scope.SCHEMA) {
+ if (Objects.requireNonNull(scope) == Scope.CATALOG
+ || Objects.requireNonNull(scope) == Scope.SCHEMA) {
Review Comment:
The scope parameter is checked with Objects.requireNonNull twice in the same
condition. Consider calling it once and storing the result, or removing the
redundant second call. For example: if (scope == Scope.CATALOG || scope ==
Scope.SCHEMA) after a single Objects.requireNonNull(scope) check at the
beginning of the method.
```suggestion
Objects.requireNonNull(scope, "scope");
if (scope == Scope.CATALOG || scope == Scope.SCHEMA) {
```
--
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]
