Skip metadata loads for table, view, and namespace existence checks - #5438
Skip metadata loads for table, view, and namespace existence checks#5438ayushtkn wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, preserves error semantics via explicit not-found exceptions, and includes targeted tests for both local and passthrough behaviors.
Pull request overview
This pull request optimizes Iceberg REST “exists” endpoints in IcebergCatalogHandler by avoiding full metadata loads when the request only needs an existence check, while still consulting remote (federated) catalogs for passthrough correctness.
Changes:
- Update
tableExists,viewExists, andnamespaceExiststo skipload*calls after successful resolution/authorization for local catalogs. - For non-local (passthrough/federated) catalogs, call
*Existson the underlying catalog and explicitly throw the appropriate “not found” exception when absent. - Add unit tests asserting local “exists” skips loads and that federated “exists” uses
*Existsand throws when the remote reports absence.
File summaries
| File | Description |
|---|---|
| runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java | Avoids expensive metadata loads for existence checks; consults passthrough catalogs via *Exists and preserves not-found semantics. |
| runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandlerTest.java | Adds coverage ensuring local existence checks don’t load metadata and passthrough existence checks call *Exists and throw when absent. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
97f7cdb to
e56a334
Compare
flyingImer
left a comment
flyingImer
left a comment
There was a problem hiding this comment.
No blocking concerns from my side. One non-blocking behavior question inline.
| if (!(baseCatalog instanceof LocalIcebergCatalog) | ||
| && !baseCatalog.tableExists(tableIdentifier)) { | ||
| throw notFoundExceptionForTableLikeEntity( |
There was a problem hiding this comment.
Is the change in metadata-table existence semantics intentional? IIRC, with Hadoop federation, tableExists can return true for a synthetic metadata table such as ns.orders.snapshots, while CatalogHandlerUtils.loadTable explicitly rejects BaseMetadataTable. http HEAD method therefore changes from 404 to 204 while GET still returns 404. If intentional, could we cover this case and note the status change in the PR description?
f21d44d to
e3a2599
Compare
| catalogHandlerUtils().loadNamespace(namespaceCatalog, namespace); | ||
| if (!(baseCatalog instanceof LocalIcebergCatalog) | ||
| && !namespaceCatalog.namespaceExists(namespace)) { | ||
| throw noSuchNamespaceException(namespace); |
There was a problem hiding this comment.
nit: the logic is correct, but a bit obscure. The fact that we delegate the existence check to namespaceCatalog is not apparent at first glance... It might be easier to read this code it had nested if statements.... with a comment about why we do not have to do the check for local catalog.
| // A federated catalog is loaded rather than asked with tableExists, because tableExists | ||
| // reports a synthetic metadata table such as ns.orders.snapshots as present while loadTable | ||
| // rejects it as not found. Loading keeps the two answers in agreement for those identifiers. | ||
| if (!(baseCatalog instanceof LocalIcebergCatalog)) { |
There was a problem hiding this comment.
Could you add a comment why loading is not needed for local catalogs?
e3a2599 to
f21d44d
Compare
Resolution already establishes existence on a local catalog, so these HEAD requests no longer load and parse metadata. Federated catalogs are asked via exists() instead, except for identifiers that could name a synthetic metadata table. Hadoop and Hive report ns.orders.snapshots as present -- HadoopCatalog inherits the default Catalog.tableExists, which loads the table and swallows the rejection, and HiveCatalog.tableExists rewrites the identifier to the base table before asking the metastore -- while loadTable rejects it as not found, so HEAD would answer 204 where GET answers 404. A metadata table always ends in a MetadataTableType name, and an ordinary table may carry one of those names too, so only those identifiers keep the load, which tells the two apart exactly as GET does.
f21d44d to
ffe20aa
Compare
tableExists,viewExistsandnamespaceExistseach resolve and authorize the entity, then load it in full and discard the result. All three methods return void — the adapter turns a normal return into a204with no body — and all three carried aResolution has already done what the endpoint asks.
resolveAndAuthorizeBasicTableLikeOperationOrThrowthrowsnotFoundExceptionForTableLikeEntitywhen the resolved leaf is absent or of the wrong subtype, so everything after it re-establishes a fact already in hand.loadTablereachesnewTableOps(ident).current()→doRefresh(), which spends two things per request:doRefreshcallsgetPassthroughResolvedPath(...), deliberately bypassing the authz resolution set the request just built, so the entity is resolved from the metastore twice.refreshFromMetadataLocation(...)→TableMetadataParser.read(fileIO, location): an object-storage GET plus a fullJSONparse of metadata that is then discarded.loadViewis the same shape throughViewMetadataParser.Much less, stated plainly.
LocalIcebergCatalog.loadNamespaceMetadatareads the already-resolved path viagetResolvedPath(...)and returns the entity's properties — no storage access, no second resolution. Removing it saves a lookup and a map copy. It is included for consistency across the three endpoints and for the federated benefit below, not for a local performance win.Federated catalogs are passthrough, so a Polaris record does not guarantee the entity is still on the remote and the remote must still be consulted. It is now asked whether the entity
existsrather than made toloadit.Against a REST-backed catalog that is a
HEADin place of aGETreturning full metadata —RESTSessionCatalog.tableExists issues client.head(...). A catalog that does not override exists falls through to theCatalog/ViewCataloginterface default, which loads and catches: the same cost as before, never worse.Because
existsreturnsfalsewhereloadthrew, the not-found is now raised explicitly —notFoundExceptionForTableLikeEntityfor tables and views,noSuchNamespaceExceptionfor namespaces — so response statuses for them stay unchanged.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)