You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Jackie-Jiang (via GitHub)" <gi...@apache.org> on 2025/11/26 03:11:45 UTC
[PR] Force colocated join when 'is_colocated_by_join_keys' is provided for semi-join [pinot]
Jackie-Jiang opened a new pull request, #17273:
URL: https://github.com/apache/pinot/pull/17273
When `is_colocated_by_join_keys` is provided, always use colocated join for semi-join.
This is consistent with the behavior for regular join, and is useful when non-partitioned keys are effectively colocated
--
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] Force colocated join when 'is_colocated_by_join_keys' is provided for semi-join [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #17273:
URL: https://github.com/apache/pinot/pull/17273#issuecomment-3578919536
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/17273?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 `85.00000%` with `3 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.57%. Comparing base ([`d78b39b`](https://app.codecov.io/gh/apache/pinot/commit/d78b39b6ca850d9aa2d50f4c47a9a92a1c8a4745?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`10a4d74`](https://app.codecov.io/gh/apache/pinot/commit/10a4d74ab515c86ac33ca30a34921914f84e8979?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/17273?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../org/apache/pinot/query/routing/WorkerManager.java](https://app.codecov.io/gh/apache/pinot/pull/17273?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fquery%2Frouting%2FWorkerManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcm91dGluZy9Xb3JrZXJNYW5hZ2VyLmphdmE=) | 77.77% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17273?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...inot/calcite/rel/logical/PinotLogicalExchange.java](https://app.codecov.io/gh/apache/pinot/pull/17273?src=pr&el=tree&filepath=pinot-query-planner%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcalcite%2Frel%2Flogical%2FPinotLogicalExchange.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2FsY2l0ZS9yZWwvbG9naWNhbC9QaW5vdExvZ2ljYWxFeGNoYW5nZS5qYXZh) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/17273?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 (d78b39b) and HEAD (10a4d74). Click for more details.
>
> <details><summary>HEAD has 36 uploads less than BASE</summary>
>
>| Flag | BASE (d78b39b) | HEAD (10a4d74) |
>|------|------|------|
>|unittests1|2|1|
>|unittests|4|1|
>|java-11|5|0|
>|temurin|10|1|
>|java-21|5|1|
>|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 #17273 +/- ##
============================================
- Coverage 63.24% 55.57% -7.67%
+ Complexity 1432 701 -731
============================================
Files 3133 2446 -687
Lines 185904 137612 -48292
Branches 28401 21888 -6513
============================================
- Hits 117568 76473 -41095
+ Misses 59277 54719 -4558
+ Partials 9059 6420 -2639
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/17273/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/17273/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/17273/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/17273/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/17273/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/17273/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/17273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <85.00%> (-7.64%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/17273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <85.00%> (-7.67%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/17273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <85.00%> (-7.67%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/17273/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.57% <85.00%> (-0.05%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/17273/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/17273?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] Force colocated join when 'is_colocated_by_join_keys' is provided for semi-join [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #17273:
URL: https://github.com/apache/pinot/pull/17273
--
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] Force colocated join when 'is_colocated_by_join_keys' is provided for semi-join [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #17273:
URL: https://github.com/apache/pinot/pull/17273#discussion_r2562762114
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotJoinToDynamicBroadcastRule.java:
##########
@@ -21,7 +21,6 @@
import org.apache.calcite.plan.RelOptRule;
import org.apache.calcite.plan.RelOptRuleCall;
import org.apache.calcite.plan.hep.HepRelVertex;
-import org.apache.calcite.rel.RelDistribution;
import org.apache.calcite.rel.RelDistributions;
Review Comment:
The import for `org.apache.calcite.rel.RelDistribution` was removed but this could cause compilation issues if `RelDistribution` type is used elsewhere in the file. Verify that all references to `RelDistribution` class are removed or that the type is available through other imports.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/routing/WorkerManager.java:
##########
@@ -146,6 +147,20 @@ private void assignWorkersToNonRootFragment(PlanFragment fragment, DispatchableP
}
}
+ private boolean isLookupJoin(List<PlanFragment> children) {
+ if (children.size() != 1) {
+ return false;
+ }
+ PlanNode planNode = children.get(0).getFragmentRoot();
+ if (!(planNode instanceof MailboxSendNode)) {
+ return false;
+ }
+ MailboxSendNode mailboxSendNode = (MailboxSendNode) planNode;
+ // NOTE: Exclude colocated semi-join which also contains a single SINGLETON exchange.
+ return mailboxSendNode.getDistributionType() == RelDistribution.Type.SINGLETON
+ && mailboxSendNode.getExchangeType() != PinotRelExchangeType.PIPELINE_BREAKER;
+ }
+
private boolean isLocalExchange(PlanFragment fragment, DispatchablePlanContext context) {
PlanNode planNode = fragment.getFragmentRoot();
if (planNode instanceof MailboxSendNode
Review Comment:
The `isLocalExchange` method appears to be unused after refactoring to use `isLookupJoin`. If this method is no longer needed, it should be removed to avoid confusion and reduce maintenance burden.
--
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