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. 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.
Still in the spirit of the spec, I think `loadCredentials` should request
credentials for `all` snapshots, even if the loadTable method doesn't actually
return them.
--
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
