eric-maynard commented on code in PR #1107:
URL: https://github.com/apache/polaris/pull/1107#discussion_r1979928173
##########
service/common/src/main/java/org/apache/polaris/service/catalog/IcebergCatalogAdapter.java:
##########
@@ -459,6 +461,25 @@ public Response listViews(
.build();
}
+ @Override
+ public Response loadCredentials(
+ String prefix,
+ String namespace,
+ String table,
+ RealmContext realmContext,
+ SecurityContext securityContext) {
+ Namespace ns = decodeNamespace(namespace);
+ TableIdentifier tableIdentifier = TableIdentifier.of(ns,
RESTUtil.decodeString(table));
+ LoadTableResponse loadTableResponse =
+ newHandlerWrapper(realmContext, securityContext, prefix)
+ .loadTableWithAccessDelegation(tableIdentifier, "all");
Review Comment:
That link is from create table, but you're right. [Here's the code pointer
for
loadTable](https://github.com/apache/polaris/blob/580e1721bf4e1531c20763e99296728eda17e897/service/common/src/main/java/org/apache/polaris/service/catalog/PolarisCatalogHandlerWrapper.java#L867).
It's probably not correct that we ignore snapshots like this (and indeed it
means changing this `all` to a `ref` would be a no-op).
Since loadCredentials doesn't accept a `snapshots`, my understanding from
the spec is that the credentials can potentially be used to read all snapshots.
So `all` should be correct here.
The difference does matter, because files in older snapshots could
potentially reside in a different location. I'm not sure how Polaris's
credential vending is supposed to work in that case however.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
