Service: Make federation logic more explicit by flyrain · Pull Request #4091 · apache/polaris

flyrain · 2026-03-31T04:35:45Z

Make federation logic more explicit instead of relying on checking on the object type. No logic change.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

flyrain · 2026-03-31T04:36:24Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

    return realmConfig().getConfig(LIST_PAGINATION_ENABLED, getResolvedCatalogEntity());
  }

-  public ListNamespacesResponse listNamespaces(


Move it down to group related methods together

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 🤷

What's the suggested action item?

singhpk234

LGTM overall ! Thanks @flyrain !

singhpk234 · 2026-03-31T05:12:19Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

+  // Indicates whether the catalog is a non-federated (Polaris-managed) catalog.
+  // Non-federated catalogs support additional capabilities such as pagination,
+  // location transformation, credential vending, and multi-table transactions.
+  private boolean isNonFederated = false;


How about we have isFederated a flag ?

or isManaged ? to have same semantics

isFederated was one of my versions. I changed to isNonFederated as there are many !isFederated. I can change it back if you feel strongly about it.

Option 1 : how about isManaged ? which means catalogs managed by Polaris, this can replace nonFederated

Option 2: also i wonder what if we filp the neg and positive cases

if (isFederated) {
// existing
} else {
// existing
}

wdyt ? isNon went hand in was a bit hard to reason, though i don't strongly feel about it, let me know what do you think or option #1 or option #2

Good idea, implemented option 2.

singhpk234

LGTM, thanks @flyrain !

flyrain · 2026-03-31T20:30:27Z

the test failure seems unrelated.

         > ERROR: failed to build: failed to solve: registry.access.redhat.com/ubi9/openjdk-21-runtime:1.24-2.1774011801: failed to resolve source metadata for registry.access.redhat.com/ubi9/openjdk-21-runtime:1.24-2.1774011801: failed to read expected number of byt
        	at io.smallrye.common.process.PipelineRunner.collectProblems(PipelineRunner.java:559)
        	at io.smallrye.common.process.ProcessRunner.complete(ProcessRunner.java:165)
        	at io.smallrye.common.process.ProcessRunner.run(ProcessRunner.java:95)
        	at io.smallrye.common.process.ProcessBuilderImpl.run(ProcessBuilderImpl.java:199)
        	at io.smallrye.common.process.ProcessBuilderImpl$ViewImpl.run(ProcessBuilderImpl.java:268)
        	at io.quarkus.container.image.docker.common.deployment.CommonProcessor.buildImage(CommonProcessor.java:337)
        	at io.quarkus.container.image.docker.deployment.DockerProcessor.createContainerImage(DockerProcessor.java:132)
        	at io.quarkus.container.image.docker.deployment.DockerProcessor.createContainerImage(DockerProcessor.java:38)
        	at io.quarkus.container.image.docker.common.deployment.CommonProcessor.buildFromJar(CommonProcessor.java:95)
        	at io.quarkus.container.image.docker.deployment.DockerProcessor.dockerBuildFromJar(DockerProcessor.java:69)
        	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
        	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:898)
        	at io.quarkus.builder.BuildContext.run(BuildContext.java:242)
        	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
        	at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2651)
        	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2630)
        	at org.jboss.threads.EnhancedQueueExecutor.runThreadBody(EnhancedQueueExecutor.java:1622)
        	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1589)
        	at java.base/java.lang.Thread.run(Thread.java:1583)
        	at org.jboss.threads.JBossThread.run(JBossThread.java:501)

dimas-b

Sorry to push back, but I hope we can improve this code a lot more with an OO approach (since we're touching it)... more specific comments below.

dimas-b · 2026-04-01T20:52:58Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

    return realmConfig().getConfig(LIST_PAGINATION_ENABLED, getResolvedCatalogEntity());
  }

-  public ListNamespacesResponse listNamespaces(


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 🤷

dimas-b · 2026-04-01T20:56:42Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

+      return catalogHandlerUtils().listNamespaces(namespaceCatalog, parent, pageToken, pageSize);
+    } else {
+      PageToken pageRequest = PageToken.build(pageToken, pageSize, this::shouldDecodeToken);
+      var results = ((IcebergCatalog) baseCatalog).listNamespaces(parent, pageRequest);


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

we now implicitly assume that non-federated catalogs are instances of IcebergCatalog

I don't understand this part. Why would we assume that now?

That assumption is in the new code - it checks the isFedeated flag, and performs an unchecked (no instanceof) cast here. This means the code assumes that baseCatalog is of type IcebergCatalog if isFedeated == false, which is based purely on ConnectionConfigInfoDpo and it is not clear how ConnectionConfigInfoDpo affects the type of baseCatalog.

The cast is required regardless, so I don’t see how this refactor makes things worse. Previously, the assumption was implicit and hidden, whereas now it’s made explicit. That’s exactly the intent of this PR.

If needed we could introduce more guardrail to ensure the catalog of federated catalogs and others.

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.

IMHO a checked cast is more explicit and easier to reason about than an if on a flag that is derived from the config, which is not directly related to the type of the variable being cast.

initializeCatalog() decides whether a catalog is federated or not. That's the reason we make it explicit there. A condition on catalog class type is hard to understand, and reasoning, given there are many different catalog classes, and all of them are Iceberg catalogs. A conditional check like if (baseCatalog instanceof IcebergCatalog polarisCatalog) makes it harder to understand whether it is a federated one or not.

initializeCatalog() decides the value of the isFederated field. It does not decide the type of baseCatalog.

If agree that the current logic in this class is not ideal. Yet, I believe changing checked casts to implicit casts is not better... that's why I'm advocating for a more holistic object-oriented approach.

dimas-b · 2026-04-01T20:58:47Z

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java

    authorizeBasicNamespaceOperationOrThrow(op, namespace);

-    if (baseCatalog instanceof IcebergCatalog polarisCatalog) {
+    if (isFederated) {


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.

Can you be more specific about more object-oriented design?

Avoid if/else blocks on the "federated" flag but use different sub-classes to implement local and federated catalogs.

The diff will be larger, of course, but I hope the overall code quality will be better.

I thought about that while making this refactor, but due to a lot of common code, I decide not to do so. But I'm open to it. With the current refactor in the this PR, it becomes easier if anyone want to do so. Is this a blocker for this refactor?

Not a blocker, but I do not really see how it improves the codebase 🤷

Service: Make federation logic more explicit

0c9b8d5

github-project-automation bot added this to Basic Kanban Board Mar 31, 2026

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 31, 2026

flyrain commented Mar 31, 2026

View reviewed changes

flyrain requested a review from singhpk234 March 31, 2026 04:37

singhpk234 reviewed Mar 31, 2026

View reviewed changes

Resovle comments

a811c23

singhpk234 previously approved these changes Mar 31, 2026

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 31, 2026

Resovle comments

bbc5225

flyrain dismissed singhpk234’s stale review via bbc5225 March 31, 2026 20:02

flyrain closed this Mar 31, 2026

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Mar 31, 2026

flyrain reopened this Mar 31, 2026

github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Mar 31, 2026

singhpk234 approved these changes Mar 31, 2026

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 31, 2026

dimas-b reviewed Apr 1, 2026

View reviewed changes

Conversation

flyrain commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

flyrain commented Mar 31, 2026

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

flyrain commented Mar 31, 2026 •

edited

Loading

singhpk234 Mar 31, 2026 •

edited

Loading

dimas-b Apr 1, 2026 •

edited

Loading

dimas-b Apr 1, 2026 •

edited

Loading

dimas-b Apr 1, 2026 •

edited

Loading

flyrain Apr 1, 2026 •

edited

Loading