jiangxt2 opened a new pull request, #11896:
URL: https://github.com/apache/gravitino/pull/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).
--
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]
