dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3028852129
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -302,6 +291,23 @@ public ListNamespacesResponse listNamespaces(Namespace
parent) {
return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent);
}
+ public ListNamespacesResponse listNamespaces(
+ Namespace parent, String pageToken, Integer pageSize) {
+ PolarisAuthorizableOperation op =
PolarisAuthorizableOperation.LIST_NAMESPACES;
+ authorizeBasicNamespaceOperationOrThrow(op, parent);
+
+ if (isFederated) {
+ return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent,
pageToken, pageSize);
+ } else {
+ PageToken pageRequest = PageToken.build(pageToken, pageSize,
this::shouldDecodeToken);
+ var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent,
pageRequest);
Review Comment:
My point is that the PR (according to its title) aims to make the "logic
more explicit", however, as far as I can tell the logic becomes less explicit.
Adding the `checkArgument` is an indication that a certain assumption is
made by this code. Such assumptions are not "explicit".
To be clear: I support improving this code. However, I do not see that
current PR is net beneficial. It converts one kind of assumption to another
kind of assumption, but the code is still obscure overall, IMHO :shrug:
I believe the best way to actually make this code more explicit is to
refactor it and separate federated catalogs logic and internal catalogs logic
into different classes. Other code areas will be affected, so it's not a small
effort, for sure.
--
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]
