You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "xiangfu0 (via GitHub)" <gi...@apache.org> on 2026/08/14 20:32:33 UTC
[PR] Add ServiceLoader-based discovery of TransformFunction implementations [pinot]
xiangfu0 opened a new pull request, #19259:
URL: https://github.com/apache/pinot/pull/19259
## Summary
Adds automatic `ServiceLoader`-based discovery of external block-oriented `TransformFunction` implementations, so an application or plugin JAR can register transform functions without setting `pinot.server.transforms` or making application-specific server startup changes.
Unlike `@ScalarFunction`s, external `TransformFunction` implementations previously could not self-register: `TransformFunctionFactory` only knew built-ins plus classes explicitly listed in `pinot.server.transforms`. Converting such functions to scalar functions is not an option when they need `ValueBlock`-level execution and batching.
## How it works
An extension JAR ships a standard service descriptor:
```
META-INF/services/org.apache.pinot.core.operator.transform.function.TransformFunction
```
listing one implementation class per line, e.g.:
```
com.example.AiFilterTransformFunction
com.example.AiClassifyTransformFunction
com.example.AiGenerateTransformFunction
```
`TransformFunctionFactory.init(...)` (called by `ServerInstance` after plugins are loaded, before query traffic) discovers providers with `ServiceLoader.load(TransformFunction.class)` on the application classpath and from every classloader returned by `PluginManager.get().getPluginClassLoaders()` — the same pattern used by `PinotRuleSet` / `OperatorTypeRegistry`. Discovery never runs on the query path; `get()` performs plain lookups against an immutable snapshot.
### Provider contract
A provider must be a public concrete class implementing `TransformFunction`, with a public no-argument constructor, returning a non-null/non-blank name from `getName()`. The startup-created instance is used only to obtain and validate the name and implementation class; it is never `init()`-ed or evaluated — query execution keeps constructing fresh instances through the factory.
### Registration and collision rules
- Built-in registrations are always preserved; names are canonicalized with `TransformFunctionFactory.canonicalize()`.
- The same implementation visible through overlapping application/plugin classloaders is de-duplicated; re-registering the identical implementation is a no-op.
- Server startup **fails** (original cause preserved) if two different discovered classes claim the same canonical name, a discovered class collides with a built-in, a service descriptor is malformed, a provider cannot be constructed, or a provider returns a null/blank name. Collision errors include the function name, both implementation classes, and their classloaders.
- `pinot.server.transforms` is preserved for backward compatibility and is applied after discovery, retaining its historical explicit-override semantics. Discovery skips classes that are also explicitly configured, so shipping a descriptor for an explicitly configured class (e.g. one that overrides a built-in name) never causes a collision failure.
- A discovered name that matches an existing scalar function is registered with a WARN (not a failure): providing a block-oriented implementation of one's own scalar function is the same pattern the built-ins use, but the log flags potential semantic divergence (literal-only invocations still constant-fold through the scalar implementation at compile time).
### Initialization and thread safety
`init(...)` is synchronized, builds and validates the registry in a local map, and atomically publishes an immutable snapshot to a `volatile` field. Readers never observe a partially initialized registry; repeated initialization is idempotent; a failed init leaves the previously published registry in place. There is no per-query or per-block discovery overhead.
### Scope
This registers functions for server-side single-stage (leaf) execution only. It does not claim automatic MSE/Calcite registration — `PinotOperatorTable` support is a separate concern. No execution-semantics changes, no annotation-based classpath scanning, no static self-registration.
## Behavior change / release note
- **New extension surface (always on, no flag):** server startup now scans the application classpath and every plugin classloader for `META-INF/services/org.apache.pinot.core.operator.transform.function.TransformFunction` descriptors. Deployments without such descriptors are completely unaffected — today no jar in the Pinot distribution ships one.
- **New fail-fast startup mode:** a malformed descriptor, a provider that cannot be constructed, a null/blank function name, or a canonical-name collision now **fails server startup** (previously such descriptors were inert because nothing loaded them). This is deliberate: silently skipping a broken provider would surface later as `Unsupported function` at query time. Operators upgrading with third-party jars that happen to ship such a descriptor should validate them; there is intentionally no disable flag, matching the `OperatorTypeDescriptor`/`RuleSetCustomizer` discovery behavior.
- **Rolling upgrade:** no wire-protocol, serialization, or config-format changes; roll-forward/roll-back safe. Explicit `pinot.server.transforms` semantics are unchanged.
- The descriptor file name (`META-INF/services/org.apache.pinot.core.operator.transform.function.TransformFunction`) becomes a permanent external contract once released; a future move of `TransformFunction` into an SPI module would need a compatibility shim for the old descriptor name.
- `getAllFunctions()` now returns an immutable snapshot current at call time (previously an unmodifiable live view); no in-repo production callers are affected, and the semantics are documented on the method.
- Follow-ups (out of scope for this focused PR): extract a shared "context classloader + plugin classloaders, dedup by class name" ServiceLoader helper into pinot-spi and migrate `PinotRuleSet`/`OperatorTypeRegistry` to it; update the transform-function docs page to mention the `META-INF/services` registration path alongside `pinot.server.transforms` (pinot-docs PR).
## Testing
- `TransformFunctionFactoryTest` (pinot-core, 21 tests): application-classpath discovery, discovery through a real `PluginManager` plugin realm, factory resolution + `ValueBlock` evaluation, case/underscore canonicalization, overlapping-classloader dedup, identical-class idempotency, discovered-vs-discovered and discovered-vs-built-in collisions, malformed descriptors, constructor failures, inaccessible constructors, null/blank names, built-ins preserved, legacy `pinot.server.transforms` registration + explicit overrides, no partial registry publication under concurrent reads, and confirmation that registration never calls `init()`/evaluation. Service descriptors are generated into temp dirs behind isolated `URLClassLoader`s so no fixture pollutes the test JVM or downstream test-jars.
- `ServerInstanceTransformFunctionTest` (pinot-server, 3 tests): a service-provided transform registers through the (extracted) `ServerInstance.initTransformFunctions` production path without `pinot.server.transforms`; the legacy config path and its missing-class failure behavior still work.
`spotless:check`, `checkstyle:check`, `license:check`, and `git diff --check` are clean on both 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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add ServiceLoader-based discovery of TransformFunction implementations [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19259:
URL: https://github.com/apache/pinot/pull/19259#issuecomment-5298255107
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19259?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `78.12500%` with `14 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 57.74%. Comparing base ([`52a4a64`](https://app.codecov.io/gh/apache/pinot/commit/52a4a64a407bc1a6f8d3783ac30417e202449231?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2a84dd2`](https://app.codecov.io/gh/apache/pinot/commit/2a84dd2a9b89ff2325badb9f59ce17df39459c53?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/19259?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...r/transform/function/TransformFunctionFactory.java](https://app.codecov.io/gh/apache/pinot/pull/19259?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Foperator%2Ftransform%2Ffunction%2FTransformFunctionFactory.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci90cmFuc2Zvcm0vZnVuY3Rpb24vVHJhbnNmb3JtRnVuY3Rpb25GYWN0b3J5LmphdmE=) | 78.12% | [10 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/19259?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19259 +/- ##
============================================
+ Coverage 57.71% 57.74% +0.03%
+ Complexity 7 1 -6
============================================
Files 2659 2656 -3
Lines 159219 159271 +52
Branches 26113 26129 +16
============================================
+ Hits 91896 91974 +78
+ Misses 59512 59491 -21
+ Partials 7811 7806 -5
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.74% <78.12%> (+0.03%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.74% <78.12%> (+0.03%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.74% <78.12%> (+0.03%)` | :arrow_up: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.74% <78.12%> (+0.03%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19259?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add ServiceLoader-based discovery of TransformFunction implementations [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19259:
URL: https://github.com/apache/pinot/pull/19259#discussion_r3788631291
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/ServerInstance.java:
##########
@@ -187,8 +199,6 @@ public ServerInstance(ServerConf serverConf, String instanceId, HelixManager hel
}
Review Comment:
The ClassNotFoundException cause is dropped when rethrowing here, which makes startup failures harder to debug (especially if the missing class name is derived/configured). Preserve the original exception as the cause so stack traces and suppressed context are retained.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add ServiceLoader-based discovery of TransformFunction implementations [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #19259:
URL: https://github.com/apache/pinot/pull/19259#discussion_r3788944229
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/ServerInstance.java:
##########
@@ -187,8 +199,6 @@ public ServerInstance(ServerConf serverConf, String instanceId, HelixManager hel
}
Review Comment:
Fixed in cfd62127c3. The transform-loading RuntimeException now preserves the ClassNotFoundException as its cause, and ServerInstanceTransformFunctionTest asserts that cause. The focused pinot-server reactor test (3 tests) plus Spotless, Checkstyle, and license checks pass.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Add ServiceLoader-based discovery of TransformFunction implementations [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19259:
URL: https://github.com/apache/pinot/pull/19259#discussion_r3810339034
##########
pinot-spi/src/main/java/org/apache/pinot/spi/plugin/PluginManager.java:
##########
@@ -503,6 +502,93 @@ public synchronized Set<ClassLoader> getPluginClassLoaders() {
return Collections.unmodifiableSet(result);
}
+ /// Discovers service providers of the given service type via `ServiceLoader`, first from the thread context
+ /// classloader (the application classpath in a standard deployment), then from every plugin classloader (see
+ /// [#getPluginClassLoaders()]), in that order.
+ ///
+ /// Providers are de-duplicated by fully-qualified class name across classloaders: overlapping classpaths (e.g.
+ /// fat-jar + plugin realm) can surface the same provider through multiple loaders, and the first sighting wins.
+ /// A same-named provider whose `Class` object differs from the first sighting indicates version skew between
+ /// classloaders; it is logged at WARN and skipped.
+ ///
+ /// Each returned entry pairs the provider instance with a human-readable description of the classloader it was
+ /// discovered from, for use in call-site log and error messages. Enumeration failures
+ /// ([ServiceConfigurationError] — malformed descriptors, provider classes that cannot be found, are not subtypes
+ /// of the service type, or fail to construct) fail fast as [IllegalStateException] carrying the source
+ /// description, with the original error preserved as the cause. Per-provider validation and registration policy
+ /// stay with the caller.
+ ///
+ /// Returns an unmodifiable, freshly computed list — never null, empty when no providers are found. Thread-safe:
+ /// each call performs a fresh enumeration using only method-local state (the plugin classloader snapshot is taken
+ /// under the existing lock), and the result reflects the calling thread's context classloader.
+ ///
+ /// Call after all plugins have been loaded; plugin classloaders registered after this call returns are not
+ /// searched.
+ public <S> List<ServiceProvider<S>> loadServiceProviders(Class<S> serviceClass) {
+ List<ServiceProvider<S>> providers = new ArrayList<>();
+ Map<String, Class<?>> seenProviderClasses = new HashMap<>();
+ collectServiceProviders(ServiceLoader.load(serviceClass), serviceClass, "thread context classloader", providers,
+ seenProviderClasses);
+ for (ClassLoader pluginClassLoader : getPluginClassLoaders()) {
+ collectServiceProviders(ServiceLoader.load(serviceClass, pluginClassLoader), serviceClass,
+ "plugin classloader: " + pluginClassLoader, providers, seenProviderClasses);
+ }
+ return Collections.unmodifiableList(providers);
+ }
+
+ private static <S> void collectServiceProviders(ServiceLoader<S> serviceLoader, Class<S> serviceClass, String source,
+ List<ServiceProvider<S>> providers, Map<String, Class<?>> seenProviderClasses) {
+ Iterator<S> iterator = serviceLoader.iterator();
+ while (true) {
+ S provider;
+ try {
+ if (!iterator.hasNext()) {
+ return;
+ }
+ provider = iterator.next();
+ } catch (ServiceConfigurationError e) {
+ throw new IllegalStateException(
+ "Failed to load a " + serviceClass.getName() + " service provider from: " + source, e);
+ }
+ Class<?> providerClass = provider.getClass();
+ String providerClassName = providerClass.getName();
+ Class<?> seenClass = seenProviderClasses.putIfAbsent(providerClassName, providerClass);
+ if (seenClass != null) {
+ // Same provider already discovered through an overlapping classloader. A different Class object with the
+ // same name indicates version skew between classloaders; the first discovered copy wins.
+ if (seenClass != providerClass) {
+ LOGGER.warn("Ignoring duplicate service provider class: {} (classloader: {}, discovered from: {}); keeping "
+ + "the copy from classloader: {}", providerClassName, providerClass.getClassLoader(), source,
+ seenClass.getClassLoader());
+ }
+ continue;
+ }
Review Comment:
`iterator.next()` constructs each provider before the class-name check below. Consequently, overlapping realms still run duplicate constructors, and a version-skewed duplicate whose constructor fails aborts startup instead of being WARNed and skipped under the documented first-sighting-wins contract. Enumerate `ServiceLoader.Provider`s, de-duplicate on `type()`, and invoke `get()` only for unseen types; a stateful/failing duplicate regression test would cover this.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org