[#11893] fix(lance): Add external table guard to deregisterTable#11896
Open
jiangxt2 wants to merge 1 commit into
Open
[#11893] fix(lance): Add external table guard to deregisterTable#11896jiangxt2 wants to merge 1 commit into
jiangxt2 wants to merge 1 commit into
Conversation
deregisterTable() delegates to dropTable(), which deletes underlying Lance data for managed tables. Add a guard that rejects non-external tables with UnsupportedOperationException, preventing silent data loss if a managed table path is ever introduced. Signed-off-by: jiangxt2 <jiangxt2@vip.qq.com>
Contributor
|
LGTM |
Code Coverage Report
Files
|
FANNG1
approved these changes
Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Add a guard in
GravitinoLanceTableOperations.deregisterTable()that rejectsnon-external (managed) tables with
UnsupportedOperationExceptionbefore callingdropTable.The guard reads
PROPERTY_EXTERNALfrom the loaded table properties using the sameOptional.ofNullable+Boolean.parseBooleanpattern as the downstreamLanceTableOperations.dropTable(), ensuring consistent behavior across both layers.Why are the changes needed?
deregisterTable()delegates todropTable(), which for managed tables deletes theunderlying 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,
deregisterTablewould 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 throwsUnsupportedOperationExceptionforexternal tables — same exception type, opposite direction (purge rejects external
tables; deregister rejects managed tables).
Fix: #11893
Does this PR introduce any user-facing change?
deregisterTablenow throwsUnsupportedOperationExceptionfor managed tables, whichLanceExceptionMappermaps to HTTP 406 (Not Acceptable). Clients would receive anexplicit 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?
testDeregisterTableRejectsManagedTablemocks a managed table (emptyproperties) and verifies:
UnsupportedOperationExceptionis throwndropTableis never called on the catalogLanceRESTServiceITcovers the external table happy path(deregister preserves physical data).