You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "walterddr (via GitHub)" <gi...@apache.org> on 2023/05/23 16:19:26 UTC
[GitHub] [pinot] walterddr opened a new pull request, #10797: [multistage][bugfix] fix singleton exchange
walterddr opened a new pull request, #10797:
URL: https://github.com/apache/pinot/pull/10797
singleton exchange was broken b/c of the worker assignment strategy.
This is a fix for instantiating singleton exchange, more to follow up properly
this change
===
- only send data via the local exchange and doesn't send data over the network I/O
- will still make broadcast of EOS block to all mailbox (regardless of whether they are local) for signaling an end of a stage)
follow up
===
- make sure it work with multiple worker on the same host (partition strategy and worker assignment)
- make mailbox assignment visitor to create proper list of send/receive based on exchange type instead of creating a fully-connected network and only send EOS over the wire for network I/O
--
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
[GitHub] [pinot] codecov-commenter commented on pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #10797:
URL: https://github.com/apache/pinot/pull/10797#issuecomment-1559876342
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/10797?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#10797](https://app.codecov.io/gh/apache/pinot/pull/10797?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ae9c7cd) into [master](https://app.codecov.io/gh/apache/pinot/commit/5577c388d0822188cb23115639becf2b3133012b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (5577c38) will **decrease** coverage by `36.46%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #10797 +/- ##
=============================================
- Coverage 68.58% 32.13% -36.46%
+ Complexity 6477 462 -6015
=============================================
Files 2159 2159
Lines 116040 116032 -8
Branches 17569 17564 -5
=============================================
- Hits 79586 37284 -42302
- Misses 30823 75444 +44621
+ Partials 5631 3304 -2327
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `23.78% <0.00%> (?)` | |
| unittests1 | `?` | |
| unittests2 | `13.67% <0.00%> (-0.01%)` | :arrow_down: |
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.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/10797?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...y/runtime/operator/BaseMailboxReceiveOperator.java](https://app.codecov.io/gh/apache/pinot/pull/10797?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9CYXNlTWFpbGJveFJlY2VpdmVPcGVyYXRvci5qYXZh) | `0.00% <ø> (-93.34%)` | :arrow_down: |
| [...ot/query/runtime/operator/MailboxSendOperator.java](https://app.codecov.io/gh/apache/pinot/pull/10797?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9NYWlsYm94U2VuZE9wZXJhdG9yLmphdmE=) | `0.00% <ø> (-86.37%)` | :arrow_down: |
| [...y/runtime/operator/exchange/SingletonExchange.java](https://app.codecov.io/gh/apache/pinot/pull/10797?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9leGNoYW5nZS9TaW5nbGV0b25FeGNoYW5nZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
... and [1441 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/10797/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10797:
URL: https://github.com/apache/pinot/pull/10797#discussion_r1202729305
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/SingletonExchange.java:
##########
@@ -41,7 +42,13 @@ class SingletonExchange extends BlockExchange {
protected void route(List<SendingMailbox> mailbox, TransferableBlock block)
throws Exception {
for (SendingMailbox sendingMailbox : mailbox) {
- sendBlock(sendingMailbox, block);
+ if (isLocal(sendingMailbox)) {
Review Comment:
TODO for this PR
- Throw when 0, or more than 1 exchange is configured ( test on block exchange unit test)
- Do not allow this ok worker assignment strategy in the first place
--
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
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10797:
URL: https://github.com/apache/pinot/pull/10797#discussion_r1203467089
##########
pinot-query-planner/src/test/resources/queries/PinotHintablePlans.json:
##########
@@ -1,6 +1,39 @@
{
"pinot_hint_option_tests": {
"queries": [
+ {
+ "description": "SELECT * inner join with filter on one table",
+ "sql": "EXPLAIN PLAN FOR SELECT /*+ joinOptions(is_colocated_by_join_keys='true') */ * FROM a JOIN b ON a.col1 = b.col2 WHERE a.col3 >= 0",
+ "output": [
+ "Execution Plan",
+ "\nLogicalJoin(condition=[=($0, $6)], joinType=[inner])",
+ "\n LogicalExchange(distribution=[single])",
+ "\n LogicalFilter(condition=[>=($2, 0)])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n LogicalExchange(distribution=[single])",
+ "\n LogicalTableScan(table=[[b]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "Inner join with group by",
Review Comment:
How is the hint been used when you have two joins and you want to singleton on just one of them or both of them?
--
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
[GitHub] [pinot] walterddr merged pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #10797:
URL: https://github.com/apache/pinot/pull/10797
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10797:
URL: https://github.com/apache/pinot/pull/10797#discussion_r1202729305
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/SingletonExchange.java:
##########
@@ -41,7 +42,13 @@ class SingletonExchange extends BlockExchange {
protected void route(List<SendingMailbox> mailbox, TransferableBlock block)
throws Exception {
for (SendingMailbox sendingMailbox : mailbox) {
- sendBlock(sendingMailbox, block);
+ if (isLocal(sendingMailbox)) {
Review Comment:
TODO for this PR
- Throw when 0, or more than 1 exchange is configured ( test on block exchange unit test)
- Do not allow this in worker assignment strategy in the first place
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java:
##########
@@ -63,7 +65,13 @@ public void onMatch(RelOptRuleCall call) {
RelNode rightExchange;
JoinInfo joinInfo = join.analyzeCondition();
- if (joinInfo.leftKeys.isEmpty()) {
+ boolean isColocatedJoin = PinotHintStrategyTable.containsHintOption(join.getHints(),
Review Comment:
done
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/SingletonExchange.java:
##########
@@ -41,7 +42,13 @@ class SingletonExchange extends BlockExchange {
protected void route(List<SendingMailbox> mailbox, TransferableBlock block)
throws Exception {
for (SendingMailbox sendingMailbox : mailbox) {
- sendBlock(sendingMailbox, block);
+ if (isLocal(sendingMailbox)) {
Review Comment:
done
--
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
[GitHub] [pinot] xiangfu0 commented on a diff in pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #10797:
URL: https://github.com/apache/pinot/pull/10797#discussion_r1203466869
##########
pinot-query-planner/src/test/resources/queries/PinotHintablePlans.json:
##########
@@ -1,6 +1,39 @@
{
"pinot_hint_option_tests": {
"queries": [
+ {
+ "description": "SELECT * inner join with filter on one table",
+ "sql": "EXPLAIN PLAN FOR SELECT /*+ joinOptions(is_colocated_by_join_keys='true') */ * FROM a JOIN b ON a.col1 = b.col2 WHERE a.col3 >= 0",
+ "output": [
+ "Execution Plan",
+ "\nLogicalJoin(condition=[=($0, $6)], joinType=[inner])",
+ "\n LogicalExchange(distribution=[single])",
+ "\n LogicalFilter(condition=[>=($2, 0)])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n LogicalExchange(distribution=[single])",
+ "\n LogicalTableScan(table=[[b]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "Inner join with group by",
Review Comment:
How is the hint been used when you have two joins and you want to singleton on just one of them or both of them?
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10797:
URL: https://github.com/apache/pinot/pull/10797#discussion_r1202730195
##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotJoinExchangeNodeInsertRule.java:
##########
@@ -63,7 +65,13 @@ public void onMatch(RelOptRuleCall call) {
RelNode rightExchange;
JoinInfo joinInfo = join.analyzeCondition();
- if (joinInfo.leftKeys.isEmpty()) {
+ boolean isColocatedJoin = PinotHintStrategyTable.containsHintOption(join.getHints(),
Review Comment:
Add extensive test for the hint config + singleton exchange (also add for dynamic broadcast join)
--
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
[GitHub] [pinot] walterddr commented on pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on PR #10797:
URL: https://github.com/apache/pinot/pull/10797#issuecomment-1559773031
CC @xiangfu0 and @ankitsultana
--
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
[GitHub] [pinot] walterddr commented on a diff in pull request #10797: [multistage][bugfix] fix singleton exchange
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #10797:
URL: https://github.com/apache/pinot/pull/10797#discussion_r1204351101
##########
pinot-query-planner/src/test/resources/queries/PinotHintablePlans.json:
##########
@@ -1,6 +1,39 @@
{
"pinot_hint_option_tests": {
"queries": [
+ {
+ "description": "SELECT * inner join with filter on one table",
+ "sql": "EXPLAIN PLAN FOR SELECT /*+ joinOptions(is_colocated_by_join_keys='true') */ * FROM a JOIN b ON a.col1 = b.col2 WHERE a.col3 >= 0",
+ "output": [
+ "Execution Plan",
+ "\nLogicalJoin(condition=[=($0, $6)], joinType=[inner])",
+ "\n LogicalExchange(distribution=[single])",
+ "\n LogicalFilter(condition=[>=($2, 0)])",
+ "\n LogicalTableScan(table=[[a]])",
+ "\n LogicalExchange(distribution=[single])",
+ "\n LogicalTableScan(table=[[b]])",
+ "\n"
+ ]
+ },
+ {
+ "description": "Inner join with group by",
Review Comment:
there's no way to do it if you write it in 2 joins. for this use case the first join needs to be written as a "WITH table"
--
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