flyrain commented on code in PR #4091:
URL: https://github.com/apache/polaris/pull/4091#discussion_r3024817085
##########
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:
Before this change, the code already relied on the fact that certain
catalogs are federated and others are not, but that assumption was hidden
inside a checked cast, so it wasn’t obvious when reading the flow.
With this refactor:
- The logic is now structured around the federation flag first. This was
done in the initialization method, which provide clearly distinguish whether a
catalog is federated or not.
- The branching makes it clear that we are handling two distinct cases
- The cast is still required, but it now happens in a well-defined branch,
instead of being buried in-line
So the assumption didn’t get introduced by this PR, it was already there.
What changed is that it is now surfaced explicitly in the control flow, making
the intent easier to see and reason about.
--
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]
