Copilot commented on code in PR #9463:
URL: https://github.com/apache/gravitino/pull/9463#discussion_r2609497256
##########
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java:
##########
@@ -291,6 +291,34 @@ public void testCreateLanceTable() {
RuntimeException.class, () ->
catalog.asTableCatalog().loadTable(newNameIdentifier));
}
+ @Test
+ void testRegisterAndDropTable() {
+ String tableName = GravitinoITUtils.genRandomName(TABLE_PREFIX);
+ NameIdentifier nameIdentifier = NameIdentifier.of(schemaName, tableName);
+ Map<String, String> properties = createProperties();
+
+ String tableLocation = tempDirectory + "/" + tableName;
+ properties.put("format", "lance");
+ properties.put("location", tableLocation);
+ properties.put("lance.register", "true");
+
+ catalog
+ .asTableCatalog()
+ .createTable(
+ nameIdentifier,
+ new Column[] {},
+ TABLE_COMMENT,
+ properties,
+ Transforms.EMPTY_TRANSFORM,
+ null,
+ null);
+
+ Table loadedTable = catalog.asTableCatalog().loadTable(nameIdentifier);
+ Assertions.assertEquals(tableName, loadedTable.name());
+
+ Assertions.assertDoesNotThrow(() ->
catalog.asTableCatalog().dropTable(nameIdentifier));
Review Comment:
The test should verify that dropping a registered (external) table does not
delete the underlying data. Consider checking if the dataset still exists at
the location after the drop operation, which is a key behavior difference for
external tables.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -259,8 +260,15 @@ Table createTableInternal(
throws NoSuchSchemaException, TableAlreadyExistsException {
if (register) {
+ // If this is a register operation, it should be a external table.
Review Comment:
The comment should clarify why registered tables must be external. Consider
expanding it to explain that registered tables should not have their underlying
data deleted when dropped, which is the behavior of external tables.
```suggestion
// If this is a register operation, it must be an external table.
// Registered tables reference existing data and should not have their
underlying data deleted
// when the table is dropped. Marking them as external ensures that
dropping the table only
// removes the metadata from Gravitino, but does not delete the actual
data at the location.
```
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -259,8 +260,15 @@ Table createTableInternal(
throws NoSuchSchemaException, TableAlreadyExistsException {
if (register) {
+ // If this is a register operation, it should be a external table.
+ Map<String, String> stringStringMap = Maps.newHashMap(properties);
+ if (!properties.containsKey(Table.PROPERTY_EXTERNAL)
+ || !Boolean.parseBoolean(properties.get(Table.PROPERTY_EXTERNAL))) {
Review Comment:
The condition logic is redundant. If the property is not present
(!properties.containsKey), then checking if it's false (!Boolean.parseBoolean)
is unnecessary because parseBoolean will return false for null/absent values.
The condition can be simplified to just check if the property is explicitly set
to true.
```suggestion
if (!Boolean.parseBoolean(properties.get(Table.PROPERTY_EXTERNAL))) {
```
##########
catalogs/catalog-lakehouse-generic/src/test/java/org/apache/gravitino/catalog/lakehouse/lance/integration/test/CatalogGenericCatalogLanceIT.java:
##########
@@ -291,6 +291,34 @@ public void testCreateLanceTable() {
RuntimeException.class, () ->
catalog.asTableCatalog().loadTable(newNameIdentifier));
}
+ @Test
+ void testRegisterAndDropTable() {
+ String tableName = GravitinoITUtils.genRandomName(TABLE_PREFIX);
+ NameIdentifier nameIdentifier = NameIdentifier.of(schemaName, tableName);
+ Map<String, String> properties = createProperties();
+
+ String tableLocation = tempDirectory + "/" + tableName;
+ properties.put("format", "lance");
+ properties.put("location", tableLocation);
+ properties.put("lance.register", "true");
+
+ catalog
+ .asTableCatalog()
+ .createTable(
+ nameIdentifier,
+ new Column[] {},
+ TABLE_COMMENT,
+ properties,
+ Transforms.EMPTY_TRANSFORM,
+ null,
+ null);
+
+ Table loadedTable = catalog.asTableCatalog().loadTable(nameIdentifier);
+ Assertions.assertEquals(tableName, loadedTable.name());
Review Comment:
The test does not verify that the 'external' property is automatically set
to "true" when 'lance.register' is true. This is the core behavior being added
in this PR. Add an assertion to check that
loadedTable.properties().get(Table.PROPERTY_EXTERNAL) equals "true".
##########
core/src/main/java/org/apache/gravitino/storage/relational/service/TableMetaService.java:
##########
@@ -289,7 +289,7 @@ public boolean deleteTable(NameIdentifier identifier) {
mapper ->
mapper.softDeletePolicyMetadataObjectRelsByTableId(
namespacedEntityId.entityId()));
- SessionUtils.doWithCommit(
+ SessionUtils.doWithoutCommit(
Review Comment:
This change appears unrelated to the PR's stated purpose of setting the
external table property during Lance table registration. The modification from
doWithCommit to doWithoutCommit changes transaction behavior in the deleteTable
method, which could affect data consistency. This should either be removed or
explained with a separate issue reference if it's an intentional fix.
##########
catalogs/catalog-lakehouse-generic/src/main/java/org/apache/gravitino/catalog/lakehouse/lance/LanceTableOperations.java:
##########
@@ -259,8 +260,15 @@ Table createTableInternal(
throws NoSuchSchemaException, TableAlreadyExistsException {
if (register) {
+ // If this is a register operation, it should be a external table.
+ Map<String, String> stringStringMap = Maps.newHashMap(properties);
Review Comment:
The variable name 'stringStringMap' is not descriptive. Consider renaming to
something more meaningful like 'updatedProperties' or 'propertiesWithExternal'
to better convey its purpose.
--
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]
