jiangxt2 opened a new issue, #11893:
URL: https://github.com/apache/gravitino/issues/11893
### Version
main branch
### Describe what's wrong
`GravitinoLanceTableOperations.deregisterTable()` (line 284) has a TODO:
"Support real deregister API." The current implementation calls
`catalog.asTableCatalog().dropTable()`, which is the same path used by
`dropTable`.
The interface contract (`LanceTableOperations.java:94-98`) defines:
- `deregisterTable`: "It will not delete the underlying lance data."
- `dropTable`: "It will delete the underlying lance data."
The downstream `LanceTableOperations.dropTable()` (catalog layer, lines
339-360) branches on `PROPERTY_EXTERNAL`:
- External tables: metadata removal only (correct for deregister)
- Managed tables: metadata removal + `dropLanceDataset()` (incorrect for
deregister)
Since all Lance REST-created tables are forced to `PROPERTY_EXTERNAL=true`
(lines 190, 221, 248), the current deregister accidentally produces correct
behavior. But it relies on this implementation coincidence rather than explicit
semantics.
### Why this matters
If a managed table path is ever introduced (e.g. Lance tables created via
Spark catalog, or a future change to allow managed Lance tables),
`deregisterTable` would silently delete physical data — a data loss bug that's
hard to diagnose because the code "looks correct" at the call site.
### Proposed fix
The `TableCatalog` interface exposes only `dropTable` and `purgeTable` — no
metadata-only path. Adding `deregisterTable` to the interface would require an
API-level change and maintainer discussion, which is out of scope for a
lightweight fix.
The pragmatic solution is a fast-failing external assertion: refuse to
deregister non-external tables rather than risk silent data loss.
In `GravitinoLanceTableOperations.deregisterTable()`, add before the
`dropTable` call (line 285):
```java
boolean external = Boolean.parseBoolean(
properties.getOrDefault(Table.PROPERTY_EXTERNAL, "false"));
if (!external) {
// deregister must NOT delete underlying data; for managed tables
dropTable would,
// and TableCatalog exposes no metadata-only path. Fail fast instead of
silently deleting.
throw new UnsupportedOperationException(
"deregisterTable only supports external tables: " + tableId);
}
// External tables: dropTable removes catalog metadata only, preserving
Lance data.
boolean result = catalog.asTableCatalog().dropTable(tableIdentifier);
```
This guard is not redundant with the downstream `dropTable` external check —
the downstream check decides *how* to drop; this assertion decides *whether* to
proceed at all.
### Testing
A unit test stubs `Table.properties()` (via the existing Mockito pattern in
`TestGravitinoLanceTableOperations`) to return a map without
`PROPERTY_EXTERNAL=true` and asserts `deregisterTable` throws
`UnsupportedOperationException`. The external happy path (metadata removed,
physical data preserved) is already covered by `LanceRESTServiceIT`.
### Future work
A proper `deregisterTable` on the `TableCatalog` interface (or a dedicated
`LanceTableOperations.deregisterTable` exposed through a Lance-specific API)
would allow metadata-only removal for any table type. This requires an
API-level design discussion with maintainers.
### Additional context
- TODO: `lance/lance-common/.../GravitinoLanceTableOperations.java:284`
- Contract: `lance/lance-common/.../LanceTableOperations.java:94-98`
- Interface: `api/.../rel/TableCatalog.java:297` (`dropTable`), `312`
(`purgeTable`)
- dropTable logic:
`catalogs/catalog-lakehouse-generic/.../LanceTableOperations.java:339-360`
--
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]
