This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 15778b188f [#11893] fix(lance): Add external table guard to
deregisterTable (#11896)
15778b188f is described below
commit 15778b188f10bbd8cd5698eb5788884611b09e0f
Author: StormSpirit <[email protected]>
AuthorDate: Mon Jul 6 09:13:22 2026 +0800
[#11893] fix(lance): Add external table guard to deregisterTable (#11896)
### What changes were proposed in this pull request?
Add a guard in `GravitinoLanceTableOperations.deregisterTable()` that
rejects
non-external (managed) tables with `UnsupportedOperationException`
before calling
`dropTable`.
The guard reads `PROPERTY_EXTERNAL` from the loaded table properties
using the same
`Optional.ofNullable` + `Boolean.parseBoolean` pattern as the downstream
`LanceTableOperations.dropTable()`, ensuring consistent behavior across
both layers.
### Why are the changes needed?
`deregisterTable()` delegates to `dropTable()`, which for managed tables
deletes the
underlying Lance dataset — violating the interface contract ("It will
not delete the
underlying lance data").
The current code is safe only because all REST-created tables are forced
to
`PROPERTY_EXTERNAL=true` (lines 190, 221, 248). This is an
implementation coincidence,
not a semantic guarantee. If a managed table path is ever introduced,
`deregisterTable`
would silently delete physical data.
The guard converts this implicit assumption into an explicit invariant:
refuse
non-external tables rather than risk silent data loss. This mirrors
`HiveCatalogOperations.purgeTable()`, which throws
`UnsupportedOperationException` for
external tables — same exception type, opposite direction (purge rejects
external
tables; deregister rejects managed tables).
Fix: #11893
### Does this PR introduce _any_ user-facing change?
`deregisterTable` now throws `UnsupportedOperationException` for managed
tables, which
`LanceExceptionMapper` maps to HTTP 406 (Not Acceptable). Clients would
receive an
explicit error instead of silent data loss. This path is currently
unreachable via the
REST API (all REST tables are external), so no existing clients are
affected.
### How was this patch tested?
- Unit test `testDeregisterTableRejectsManagedTable` mocks a managed
table (empty
properties) and verifies:
- `UnsupportedOperationException` is thrown
- `dropTable` is never called on the catalog
- Existing integration test `LanceRESTServiceIT` covers the external
table happy path
(deregister preserves physical data).
Signed-off-by: jiangxt2 <[email protected]>
---
.../gravitino/GravitinoLanceTableOperations.java | 16 +++++++++-
.../rest/TestGravitinoLanceTableOperations.java | 34 ++++++++++++++++++++++
2 files changed, 49 insertions(+), 1 deletion(-)
diff --git
a/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
b/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
index cad7735118..50ba09df30 100644
---
a/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
+++
b/lance/lance-common/src/main/java/org/apache/gravitino/lance/common/ops/gravitino/GravitinoLanceTableOperations.java
@@ -281,7 +281,21 @@ public class GravitinoLanceTableOperations implements
LanceTableOperations {
"Table not found: " + tableId, CommonUtil.formatCurrentStackTrace(),
tableId);
}
Map<String, String> properties = t.properties();
- // TODO Support real deregister API.
+
+ // Verify the table is external before deregistering. For non-external
(managed) tables,
+ // dropTable would delete the underlying Lance data, violating the
deregister contract
+ // ("It will not delete the underlying lance data"). The TableCatalog
interface does not
+ // expose a metadata-only removal path, so we fail fast rather than risk
silent data loss.
+ boolean external =
+ Optional.ofNullable(properties.get(Table.PROPERTY_EXTERNAL))
+ .map(Boolean::parseBoolean)
+ .orElse(false);
+ if (!external) {
+ throw new UnsupportedOperationException(
+ "deregisterTable only supports external tables, but table " +
tableId + " is managed");
+ }
+
+ // External tables: dropTable removes catalog metadata only, preserving
Lance data.
boolean result = catalog.asTableCatalog().dropTable(tableIdentifier);
if (!result) {
throw new TableNotFoundException(
diff --git
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestGravitinoLanceTableOperations.java
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestGravitinoLanceTableOperations.java
index 10c9b6bf32..84618a0932 100644
---
a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestGravitinoLanceTableOperations.java
+++
b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/rest/TestGravitinoLanceTableOperations.java
@@ -21,11 +21,17 @@ package org.apache.gravitino.lance.service.rest;
import static
org.apache.gravitino.lance.common.utils.LanceConstants.LANCE_TABLE_VERSION;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.NameIdentifier;
+import
org.apache.gravitino.lance.common.ops.gravitino.GravitinoLanceNamespaceWrapper;
import
org.apache.gravitino.lance.common.ops.gravitino.GravitinoLanceTableAlterHandler.AlterColumnsGravitinoLance;
import
org.apache.gravitino.lance.common.ops.gravitino.GravitinoLanceTableAlterHandler.DropColumns;
+import
org.apache.gravitino.lance.common.ops.gravitino.GravitinoLanceTableOperations;
import org.apache.gravitino.rel.Table;
+import org.apache.gravitino.rel.TableCatalog;
import org.apache.gravitino.rel.TableChange;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -92,4 +98,32 @@ class TestGravitinoLanceTableOperations {
Assertions.assertEquals(
"Only RENAME alteration is supported currently.",
exception.getMessage());
}
+
+ @Test
+ void testDeregisterTableRejectsManagedTable() {
+ // Mock a managed table (no PROPERTY_EXTERNAL=true)
+ Table managedTable = Mockito.mock(Table.class);
+ Mockito.when(managedTable.properties()).thenReturn(new HashMap<>());
+
+ TableCatalog tableCatalog = Mockito.mock(TableCatalog.class);
+ Mockito.when(tableCatalog.loadTable(Mockito.any(NameIdentifier.class)))
+ .thenReturn(managedTable);
+
+ Catalog catalog = Mockito.mock(Catalog.class);
+ Mockito.when(catalog.asTableCatalog()).thenReturn(tableCatalog);
+
+ GravitinoLanceNamespaceWrapper wrapper =
Mockito.mock(GravitinoLanceNamespaceWrapper.class);
+
Mockito.when(wrapper.loadAndValidateLakehouseCatalog(Mockito.anyString())).thenReturn(catalog);
+
+ GravitinoLanceTableOperations ops = new
GravitinoLanceTableOperations(wrapper);
+
+ UnsupportedOperationException exception =
+ Assertions.assertThrows(
+ UnsupportedOperationException.class,
+ () -> ops.deregisterTable("catalog.schema.table", "."));
+ Assertions.assertTrue(exception.getMessage().contains("only supports
external tables"));
+
+ // Verify dropTable was never called — the guard must reject before
reaching the catalog layer.
+ Mockito.verify(tableCatalog, Mockito.never()).dropTable(Mockito.any());
+ }
}
