Modularize calls to federated catalogs #2301

poojanilangekar · 2025-08-07T23:35:36Z

Based on the discussion in the community sync, this PR modularizes federation calls to a separate factory. The specific instances of the NonRESTCatalogFactory is loaded only when a non-REST catalog is initialized and accessed. Beyond the catalog initialization, all the calls are identical for internal and federated catalogs.

After this change, Polaris needs to be compiled with the NonRESTCatalogs property set to the list of catalogs for which are implemented/supported. Unless a catalog type is a part of the flag, Polaris will not load its implementation at runtime.

Testing:
Applied the regtest in PR #2286.

poojanilangekar · 2025-08-07T23:35:55Z

CC @eric-maynard @dennishuo

…ntimOnly

poojanilangekar · 2025-08-09T00:08:30Z

The conditional runtime dependency could rely on one of the two options:

Based on the NonRESTCatalogs property (which an additional property to be set in gradle.properties or as a JVM argument).
Reuse the existing polaris.features.SUPPORTED_EXTERNAL_CATALOG_AUTHENTICATION_TYPES but require the user to declare it in gradle.properties instead of application.properties.

The current implementation uses option 1.

I can change the option or name if necessary. If you have other possible suggestions, please let me know.

polaris-core/src/main/java/org/apache/polaris/core/catalog/NonRESTCatalogFactory.java

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

eric-maynard

LGTM overall, just one concern about how we can unify the approach taken for IRC and non-IRC federation

snazy

Looks like this PR introduces a breaking change that removes functionality.

snazy · 2025-08-12T10:57:02Z

runtime/service/build.gradle.kts

  implementation(project(":polaris-api-management-service"))
  implementation(project(":polaris-api-iceberg-service"))
  implementation(project(":polaris-api-catalog-service"))
+  if ((project.findProperty("NonRESTCatalogs") as String?)?.contains("HADOOP") == true) {


This is a breaking change to Polaris that effectively removes functionality in the Polaris server.

IIUC the functionality is still there, but the way you need to build Polaris to use that functionality does change (across versions).

In any case, shouldn't this block be declared rather in runtime/server?

Regarding runtime/server module: We reference the ExternalCatalogFactory in the IcebergCatalogHandler which is in runtime/service module. So isn't this the correct location?

But afaict ExternalCatalogFactory is an interface in polaris-core. We don't reference HadoopFederatedCatalogFactory directly in code.

In runtime/server we already have other similar runtimeOnly declarations:

runtimeOnly(project(":polaris-eclipselink")) runtimeOnly("org.postgresql:postgresql") runtimeOnly(project(":polaris-relational-jdbc")) runtimeOnly("io.quarkus:quarkus-jdbc-postgresql")

I had that until c73966c

I can do either, from the Polaris OSS sync last week (discussion about regarding hive federation), my takeaway was that we wanted to avoid having the default Polaris JAR depend on anything hadoop.

However, if you'd prefer compiling it each time (and only loading if necessary), I can revert that change. I will send out a separate PR without dynamic compilation and update a README.md for this PR. Please pick the option that's best suited according to you.

+1 to adding something to the README/getting-started or similar to better make it an easy copy/paste command to decide whether to compile with or without the extended dependencies.

My understanding is indeed that this check was directly to address the concern others had about having Hadoop (or Hive in the future) compile-time dependencies be always present for all Polaris builds.

Personally I don't feel too strongly either way, so I'm okay with or without the additional compilation property.

Also, per offline discussion, +1 to what @adutra said about putting the Hadoop extension into runtime/server if it works correctly for Quarkus finding it in the runtime assembly.

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

adutra · 2025-08-12T16:49:38Z

extensions/federation/hadoop/build.gradle.kts

+java { toolchain { languageVersion.set(JavaLanguageVersion.of(21)) } }
+
+tasks.withType<JavaCompile> { options.encoding = "UTF-8" }
+
+tasks.test { useJUnitPlatform() }


These lines shouldn't be required.

adutra · 2025-08-12T16:57:15Z

runtime/service/build.gradle.kts

  implementation(project(":polaris-api-management-service"))
  implementation(project(":polaris-api-iceberg-service"))
  implementation(project(":polaris-api-catalog-service"))
+  if ((project.findProperty("NonRESTCatalogs") as String?)?.contains("HADOOP") == true) {


In any case, shouldn't this block be declared rather in runtime/server?

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

dennishuo · 2025-08-12T20:59:24Z

runtime/service/build.gradle.kts

  implementation(project(":polaris-api-management-service"))
  implementation(project(":polaris-api-iceberg-service"))
  implementation(project(":polaris-api-catalog-service"))
+  if ((project.findProperty("NonRESTCatalogs") as String?)?.contains("HADOOP") == true) {


+1 to adding something to the README/getting-started or similar to better make it an easy copy/paste command to decide whether to compile with or without the extended dependencies.

My understanding is indeed that this check was directly to address the concern others had about having Hadoop (or Hive in the future) compile-time dependencies be always present for all Polaris builds.

Personally I don't feel too strongly either way, so I'm okay with or without the additional compilation property.

dennishuo · 2025-08-12T21:00:45Z

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

+      Instance<ExternalCatalogFactory> externalCatalogFactory =
+          externalCatalogFactories.select(
+              Identifier.Literal.of(connectionType.getFactoryIdentifier()));
+      if (!externalCatalogFactory.isUnsatisfied()) {


Generally I like to avoid double-negatives where possible, so could be worth considering swapping the if/else here to get rid of the double-negative.

Done, I've used isResolvable() instead.

snazy

Sorry, my point that this is a breaking change as it removes functionality from Apache Polaris still stands.

Nothing ever sets this Gradle property.

Projects MUST direct outsiders towards official releases rather than raw source repositories, nightly builds, snapshots, release candidates, or any other similar packages.. Asking users to re-build Polaris from source means that that have to be active developers and are "... aware of the conditions placed on unreleased materials."

Yes, "nothing Hadoop" should end up in Polaris, and that should be the eventual goal.
But removing existing user facing functionality deserves a broader audience, deprecation and eventual removal.

poojanilangekar · 2025-08-13T17:37:54Z

@snazy I've sent out an alternate in #2332 which does not remove the functionality by default but rather creates a separate module that can be removed at a later point. If you oppose removing the functionality, then simply modularizing it makes no user visible changes. We need one of these two options to move forward with Hive federation (which I've proposed over a month ago and we agreed upon during the last community sync). Hence I request you to pick one of the two options so we can make progress on supporting various federation options for our users.

Modularize calls to federated catalogs

afe30fa

github-project-automation bot added this to Basic Kanban Board Aug 7, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Aug 7, 2025

poojanilangekar added 7 commits August 7, 2025 16:47

Remove verbose logs

63f25b1

Separate modules per non-irc catalog

5cafd63

Merge remote-tracking branch 'origin/main' into federation-module

d9cdb71

spotlesApply

91e1807

Modified the polaris-extensions-federation-hadoop dependency to by ru…

e036e9f

…ntimOnly

Rename

2c4aa58

Make the runtime dependency flag dependent

c73966c

spotlessApply

284ad40

eric-maynard reviewed Aug 11, 2025

View reviewed changes

polaris-core/src/main/java/org/apache/polaris/core/catalog/NonRESTCatalogFactory.java Outdated Show resolved Hide resolved

eric-maynard reviewed Aug 11, 2025

View reviewed changes

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java Outdated Show resolved Hide resolved

eric-maynard reviewed Aug 11, 2025

View reviewed changes

Address review comments

790f81b

snazy reviewed Aug 12, 2025

View reviewed changes

eric-maynard reviewed Aug 12, 2025

View reviewed changes

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java Outdated Show resolved Hide resolved

adutra reviewed Aug 12, 2025

View reviewed changes

Address review comments

05dbe0c

eric-maynard reviewed Aug 12, 2025

View reviewed changes

.../service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java Outdated Show resolved Hide resolved

Remove the switch statement

6e78c72

eric-maynard previously approved these changes Aug 12, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Aug 12, 2025

poojanilangekar mentioned this pull request Aug 12, 2025

Modularize federation (Option 2) #2332

Merged

dennishuo previously approved these changes Aug 12, 2025

View reviewed changes

Add a README

7c7e257

poojanilangekar dismissed stale reviews from dennishuo and eric-maynard via 7c7e257 August 12, 2025 21:05

poojanilangekar added 3 commits August 12, 2025 14:11

Remove double negative in factory resolution check

4fb7967

Update the README

1cbafb3

Update README.md

1902d21

dennishuo approved these changes Aug 12, 2025

View reviewed changes

dennishuo previously approved these changes Aug 12, 2025

View reviewed changes

Add license to README

103fe6f

poojanilangekar dismissed dennishuo’s stale review via 103fe6f August 12, 2025 22:26

eric-maynard approved these changes Aug 12, 2025

View reviewed changes

snazy requested changes Aug 13, 2025

View reviewed changes

github-project-automation bot moved this from Ready to merge to PRs In Progress in Basic Kanban Board Aug 13, 2025

poojanilangekar closed this Aug 14, 2025

github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Aug 14, 2025

Modularize calls to federated catalogs #2301

Modularize calls to federated catalogs #2301

Uh oh!

Conversation

poojanilangekar commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poojanilangekar commented Aug 7, 2025

Uh oh!

poojanilangekar commented Aug 9, 2025

Uh oh!

Uh oh!

Uh oh!

eric-maynard left a comment

Choose a reason for hiding this comment

Uh oh!

snazy 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!

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!

dennishuo Aug 12, 2025 • 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!

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!

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!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

poojanilangekar commented Aug 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

poojanilangekar commented Aug 7, 2025 •

edited

Loading

dennishuo Aug 12, 2025 •

edited

Loading