You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "flyrain (via GitHub)" <gi...@apache.org> on 2026/04/03 00:54:34 UTC
[PR] Move CallContextCatalogFactory to core module and rename it [polaris]
flyrain opened a new pull request, #4117:
URL: https://github.com/apache/polaris/pull/4117
Polaris currently uses external catalog for two different ideas: the broad concept of catalogs whose source of truth lives outside Polaris, and the narrower runtime path where Polaris builds a remote catalog client from ConnectionConfigInfo and delegates operations to it. That makes the current SPI name misleading, since notification-synced catalogs are also external in the broad sense but do not use this factory. Likewise, CallContextCatalogFactory describes an implementation detail rather than the role of the abstraction.
This PR makes that split explicit by renaming the remote delegation path to FederatedCatalogFactory and the Polaris-managed path to LocalCatalogFactory. That gives us clearer terminology: external stays the umbrella concept, federated refers to remote-backed execution, and local refers to the in-process Polaris-backed path. No behavioral change is intended.
This PR also moves the local catalog factory interface into polaris-core. The contract depends only on core concepts, and keeping it in core should make future SPI enforcement simpler if we treat interfaces in polaris-core as the supported SPI surface.
## 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)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3033660138
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
> It’s because the interface itself is a contract/SPI boundary, and that contract is expressed entirely in core types (PolarisResolutionManifest).
I agree that this interface is an SPI, however, it is not a "core SPI" in the sense that Polaris Core does not need an implementation of this SPI to be provided in order to function property.
If the idea behind moving it to `polaris-core` is only to consolidate all SPI classes in `polaris-core`, I'd like to propose postponing the move until we make progress on the general SPI standardization discussion (which we started on `dev` a long time ago, but did not progress, unfortunately).
I imagine different components of Polaris may have different SPI surfaces. In this case `LocalCatalogFactory` is required by the `service` code, so keeping it under `runtime/service` seems more natural to me.
If we were to formalize service SPI, I'd suggest adding a dedicated `runtime/service-spi` module instead of moving to `polaris-core`. That would lead to better isolation and dependency coherence among modules.
My view of SPI is that it should be strongly associated with the module that calls it in runtime, not with the module that defines its method parameter types.
In any case, as I noted above, I hope for some consensus on the SPI standardization discussion before moving classes across modules.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "gh-yzou (via GitHub)" <gi...@apache.org>.
gh-yzou commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3034871388
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
I think the current change is fine, at least it groups the two catalog related factory interface together.
I agree with @dimas-b 's point about accelerate the SPI discussion, we should continue push the discussion forward and come up with clear guidance about where to put those interface definitions
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3033697347
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
Re: `FederatedCatalogFactory`, I believe it also belongs with the "service" SPI, not "core" SPI.
I suppose the only reason it is defined in `polaris-core` is to allow simpler reuse in exception modules (Hadoop, Hive).
IMHO, this only emphasizes the need to accelerate the SPI discussion and cleanup module dependencies with a coherent SPI definition (modularization) strategy.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3031014602
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
Here are two reasons I moved it:
1. Be consistent with the `FederatedCatalogFactory` in https://github.com/apache/polaris/pull/4116, `core` owns the abstraction, `service/extensions` own the implementations.
2. It isn’t because `polaris-core` needs to call it directly. It’s because the interface itself is a contract/SPI boundary, and that contract is expressed entirely in core types (`PolarisResolutionManifest`).
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3033345715
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
import org.apache.iceberg.catalog.Catalog;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
-public interface CallContextCatalogFactory {
+public interface LocalCatalogFactory {
Review Comment:
Thanks a lot ! It's all good to me.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain merged PR #4117:
URL: https://github.com/apache/polaris/pull/4117
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "HonahX (via GitHub)" <gi...@apache.org>.
HonahX commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3034908454
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
+1 on current change and the formal SPI discussion. I think it will be good to have a clearer guidance on how and where to put interfaces for factories and other beans.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3030994324
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
Why move it to core? None of the core classes need to reference this factory 🤔
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3033697347
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
Re: `FederatedCatalogFactory`, I believe it also belongs with the "service" SPI, not "core" SPI.
I suppose the only reason it is defined in `polaris-core` is to allow simpler reuse in extension modules (Hadoop, Hive).
IMHO, this only emphasizes the need to accelerate the SPI discussion and cleanup module dependencies with a coherent SPI definition (modularization) strategy.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3035220229
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
Totally agree that we should move forward with the SPI discussion and provide clear guidance for the interface. Thanks every one for chiming one. cc @flyingImer.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module [polaris]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3031665722
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
import org.apache.iceberg.catalog.Catalog;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
-public interface CallContextCatalogFactory {
+public interface LocalCatalogFactory {
Review Comment:
nit: I was a bit confused by the PR title and the actual change - The PR title says "Move CallContextCatalogFactory to core module" but it also renames to `LocalCatalogFactory`. The title should reflect both the move and the rename.
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
`PolarisLocalCatalogFactory` stays in `org.apache.polaris.service.context.catalog` while the interface moved to `org.apache.polaris.core.catalog`.
This is fine (implementation in service, SPI in core), but the old package `context.catalog` now only has `PolarisLocalCatalogFactory` and `RealmContextHolder` — worth noting if future cleanup consolidates packages.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3033677866
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
Furthermore, having `LocalCatalogFactory` in `polaris-core` overloads core with classes that have no meaning within the scope of the `polaris-core` module. `LocalCatalogFactory` has meaning only in `runtime/service`.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3033317128
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
import org.apache.iceberg.catalog.Catalog;
import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
-public interface CallContextCatalogFactory {
+public interface LocalCatalogFactory {
Review Comment:
updated the title
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Move CallContextCatalogFactory to core module and rename [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4117:
URL: https://github.com/apache/polaris/pull/4117#discussion_r3033753724
##########
polaris-core/src/main/java/org/apache/polaris/core/catalog/LocalCatalogFactory.java:
##########
@@ -16,12 +16,12 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.polaris.service.context.catalog;
+package org.apache.polaris.core.catalog;
Review Comment:
Yet, I do not insist... this move in current PR does not make the overall situation worse. I still maintain my points (above), but feel free to merge with the move.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org