shaoyu-li commented on PR #13058:
URL: https://github.com/apache/gravitino/pull/13058#issuecomment-5669176745
Thanks -- reading this against `LanceTableOperations`, `IsolatedClassLoader`
and the rest of the
module is what made it land, and it found things three passes over the diff
alone had not. All nine
inline points checked out against the code; all nine are addressed in
8ffd3c2. Details in the
threads, but three are worth repeating here:
- **Purge of an external table** was the serious one, and it was mine: the
external skip I added
last round meant Lance deleted the dataset and the allocation was never
handed back, in the same
call. A purge now unprovisions regardless of `external`, and the context
exposes `isPurge()`.
- **A caller-supplied location now reaches the provider.** Your dead-branch
observation settled it:
the built-in provider's first branch was unreachable, which is the code
making your argument for
you. Unchanged for catalogs on the built-in provider, which returns such a
location verbatim.
- **The per-class-loader index is gone.** `isCatalogClass` matches this
package, so the map was
keyed by the loader that owned it and never held more than one entry.
Also taken: the format cache invalidated in a `finally` with the location
named in a WARN, the null
tolerance finished on both halves rather than claimed by a test that only
held for the fake
delegator, `loadSchema` moved inside the cascade guard, the purge/drop
wording in the contract, the
404 -> 200 change written up in the docs, and the table alignment.
On the two non-blocking test points: the description no longer claims a drop
reads the entity once
overall -- it says the catalog reads it once and that Lance's own
`loadTable` is below this SPI. I
have not added the real-Lance end-to-end test; the behaviour it would pin on
the Lance side is
Lance's own and already covered there, and the catalog-side rule that
follows from it is pinned
against the fake delegator. Say the word if you would rather have it anyway.
94 tests in the module, 0 failures, 0 skipped. Every behavioural fix here
was checked by reverting
it and confirming the matching test fails.
One request: the workflows on the previous commits sat at `action_required`
until someone approved
them. If the same happens to 8ffd3c2, could you approve the run?
--
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]
