You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "gortiz (via GitHub)" <gi...@apache.org> on 2026/03/25 14:34:33 UTC

[PR] Add OpChain converter SPI and adopt it in runtime paths. [pinot]

gortiz opened a new pull request, #17974:
URL: https://github.com/apache/pinot/pull/17974

   ## Summary
   - Introduces an OpChain conversion SPI so Pinot can support alternative plan-to-OpChain implementations.
   - Adds a ServiceLoader-based dispatcher that selects the active converter (by priority or explicit override).
   - Migrates runtime conversion call sites to use the dispatcher, so the SPI is used in production execution paths.
   
   ## Why
   Pinot previously used a single hardcoded converter (`PlanNodeToOpChain`) in runtime execution paths. To enable extensibility (for example, alternative execution backends) without forking core runtime flow, we need a first-class conversion extension point and centralized dispatch. For example this can be a first step on the idea of fusing the timeseries engine with MSE.
   
   ## Changes Included (with effect)
   
   - `pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/OpChainConverter.java`
     - Added new SPI interface for converting `PlanNode` to `OpChain`.
     - Added default implementation (`DefaultOpChainConverter`) that delegates to existing `PlanNodeToOpChain`.
     - Registered default implementation via `@AutoService`.
       - **Effect:** introduces an extension point while preserving current behavior as the default path.
   
   - `pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/OpChainConverterDispatcher.java`
     - Added dispatcher that discovers converters via `ServiceLoader`.
     - Added selection logic:
       - highest `priority()` wins by default
       - explicit converter override supported via `setActiveConverterIdOverride(...)`
       - deterministic tie-break by converter id
     - Added optimized fast path for default converter to avoid overhead on stock path.
       - **Effect:** centralizes converter selection and runtime dispatch, enabling pluggable conversion implementations.
   
   - `pinot-query-runtime/src/test/java/org/apache/pinot/query/runtime/plan/OpChainConverterDispatcherTest.java`
     - Added tests for converter registration/selection/override behavior.
       - **Effect:** validates SPI dispatch behavior and prevents regressions in converter resolution.
   
   - `pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/QueryRunner.java`
     - Replaced direct `PlanNodeToOpChain.convert(...)` call with dispatcher conversion.
       - **Effect:** non-leaf stage execution now honors active converter SPI.
   
   - `pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/pipeline/PipelineBreakerExecutor.java`
     - Replaced direct `PlanNodeToOpChain.convert(...)` with dispatcher conversion.
       - **Effect:** pipeline breaker execution now uses the active converter SPI.
   
   - `pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/server/ServerPlanRequestUtils.java`
     - Replaced direct `PlanNodeToOpChain.convert(...)` with dispatcher conversion.
       - **Effect:** server-side leaf stage compile path now uses the active converter SPI.
   
   - `pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java`
     - Replaced reduce-stage conversion call to use dispatcher conversion.
       - **Effect:** broker reducer conversion path now also uses active converter SPI, keeping conversion behavior consistent across runtime entry points.
   
   ## Behavior / Compatibility
   - Default behavior remains unchanged because the default SPI implementation delegates to `PlanNodeToOpChain`.
   - No query semantics change is intended for existing deployments.
   - New functionality is additive: custom converters can be introduced as separate implementations discovered via `ServiceLoader`.
   
   ## Test Plan
   - Unit tests:
     - `./mvnw -pl pinot-query-runtime -Dtest=OpChainConverterDispatcherTest test`
   - Focused compile/tests for affected module:
     - `./mvnw -pl pinot-query-runtime -am -DskipTests compile`
     - `./mvnw -pl pinot-query-runtime -am test -Dtest=QueryRunnerTest`
   - Optional smoke:
     - Run a representative multistage query and verify execution/results are unchanged with default converter.


-- 
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 OpChain converter SPI and adopt it in runtime paths. [pinot]

Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz merged PR #17974:
URL: https://github.com/apache/pinot/pull/17974


-- 
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 OpChain converter SPI and adopt it in runtime paths. [pinot]

Posted by "yashmayya (via GitHub)" <gi...@apache.org>.
yashmayya commented on code in PR #17974:
URL: https://github.com/apache/pinot/pull/17974#discussion_r2993648487


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/plan/OpChainConverter.java:
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.query.runtime.plan;
+
+import com.google.auto.service.AutoService;
+import java.util.function.BiConsumer;
+import org.apache.pinot.query.planner.plannode.PlanNode;
+import org.apache.pinot.query.runtime.operator.MultiStageOperator;
+import org.apache.pinot.query.runtime.operator.OpChain;
+
+
+/**
+ * Extension point for alternative plan-to-{@link OpChain} converters. The active implementation is chosen by
+ * {@link OpChainConverterDispatcher}: highest {@link #priority()} wins unless an override is set (for product
+ * extensions that pin a specific converter at process startup).
+ */
+public interface OpChainConverter {
+  /**
+   * Returns the converter identifier handled by this provider (case-insensitive for lookup; typically lowercase).
+   */
+  String converterId();
+
+  /**
+   * Relative preference when more than one converter is registered. The converter with the largest value is used
+   * unless {@link OpChainConverterDispatcher#setActiveConverterIdOverride} is set. Ties break lexicographically by
+   * {@link #converterId()} (ascending).
+   *
+   * It is recommended to use multiples of 1000 for the priority, as that gives more flexibility to other
+   * converters to be registered with different priorities.
+   */
+  default int priority() {
+    return 0;
+  }
+
+  /**
+   * Converts the plan node into an opchain for this converter.
+   */
+  OpChain convert(PlanNode node, OpChainExecutionContext context, BiConsumer<PlanNode, MultiStageOperator> tracker);

Review Comment:
   Might be worth adding a small note about the context and tracker arguments here?



-- 
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 OpChain converter SPI and adopt it in runtime paths. [pinot]

Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on PR #17974:
URL: https://github.com/apache/pinot/pull/17974#issuecomment-4136261868

   Documentation for this feature has been added in pinot-contrib/pinot-docs#581.
   
   The OpChainConverter SPI documentation includes:
   - Overview of the new SPI interface and its use cases
   - Implementation guide with code examples
   - Registration via @AutoService and Java ServiceLoader
   - Priority-based selection mechanism
   - Example custom converter implementation
   
   See: https://github.com/pinot-contrib/pinot-docs/pull/581


-- 
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 OpChain converter SPI and adopt it in runtime paths. [pinot]

Posted by "gortiz (via GitHub)" <gi...@apache.org>.
gortiz commented on PR #17974:
URL: https://github.com/apache/pinot/pull/17974#issuecomment-4144653186

   I don't think this feature needs to be documented, but it won't hurt


-- 
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 OpChain converter SPI and adopt it in runtime paths. [pinot]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17974:
URL: https://github.com/apache/pinot/pull/17974#issuecomment-4127678930

   ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17974?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 `70.90909%` with `16 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 55.51%. Comparing base ([`5b7d6fd`](https://app.codecov.io/gh/apache/pinot/commit/5b7d6fd146e655aba05e2dd9625df516e39d23d6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`aee3815`](https://app.codecov.io/gh/apache/pinot/commit/aee38159ba235f413fbeb4a4ab58dbc7c782e242?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   :warning: Report is 12 commits behind head on master.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/17974?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
   |---|---|---|
   | [...query/runtime/plan/OpChainConverterDispatcher.java](https://app.codecov.io/gh/apache/pinot/pull/17974?src=pr&el=tree&filepath=pinot-query-runtime%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Fruntime%2Fplan%2FOpChainConverterDispatcher.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9wbGFuL09wQ2hhaW5Db252ZXJ0ZXJEaXNwYXRjaGVyLmphdmE=) | 65.95% | [9 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17974?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
   > :exclamation:  There is a different number of reports uploaded between BASE (5b7d6fd) and HEAD (aee3815). Click for more details.
   > 
   > <details><summary>HEAD has 36 uploads less than BASE</summary>
   >
   >| Flag | BASE (5b7d6fd) | HEAD (aee3815) |
   >|------|------|------|
   >|java-21|5|1|
   >|unittests1|2|1|
   >|unittests|4|1|
   >|temurin|10|1|
   >|java-11|5|0|
   >|unittests2|2|0|
   >|integration|6|0|
   >|integration2|2|0|
   >|integration1|2|0|
   >|custom-integration1|2|0|
   ></details>
   
   <details><summary>Additional details and impacted files</summary>
   
   
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #17974      +/-   ##
   ============================================
   - Coverage     63.30%   55.51%   -7.79%     
   + Complexity     1525      746     -779     
   ============================================
     Files          3193     2500     -693     
     Lines        193330   142736   -50594     
     Branches      29732    22886    -6846     
   ============================================
   - Hits         122381    79245   -43136     
   + Misses        61356    56747    -4609     
   + Partials       9593     6744    -2849     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [integration](https://app.codecov.io/gh/apache/pinot/pull/17974/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/17974/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/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-11](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   | [java-21](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.51% <70.90%> (-7.74%)` | :arrow_down: |
   | [temurin](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.51% <70.90%> (-7.79%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.51% <70.90%> (-7.78%)` | :arrow_down: |
   | [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.51% <70.90%> (-0.01%)` | :arrow_down: |
   | [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17974/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
   
   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 Sentry](https://app.codecov.io/gh/apache/pinot/pull/17974?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