dimas-b commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024612334
##########
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:
I would not call this "more explicit logic". IMHO, the logic becomes less
explicit because instead of using a checked cast (line 239 before) we now
implicitly assume that non-federated catalogs are instances of `IcebergCatalog`
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -381,15 +387,15 @@ public ListTablesResponse listTables(Namespace namespace,
String pageToken, Inte
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.LIST_TABLES;
authorizeBasicNamespaceOperationOrThrow(op, namespace);
- if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+ if (isFederated) {
Review Comment:
I'd prefer a different kind of refactoring for this code so as to remove
these if/else constructs and have a more object-oriented design.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -231,23 +235,6 @@ private boolean shouldDecodeToken() {
return realmConfig().getConfig(LIST_PAGINATION_ENABLED,
getResolvedCatalogEntity());
}
- public ListNamespacesResponse listNamespaces(
Review Comment:
nit: moving code is fine, but it's outside the declared scope of this PR
(per PR description) and makes the "real diff" harder to deduce 🤷
--
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]
