You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/12/07 05:02:48 UTC
[GitHub] [pinot] 61yao opened a new pull request, #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
61yao opened a new pull request, #9930:
URL: https://github.com/apache/pinot/pull/9930
1) This can can be used to check how much performance improvement skip shuffle brings
2) It can also be used to quickly check whether skip shuffle introduces bugs
--
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] 61yao commented on pull request #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
61yao commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341738470
> let's remove all skipShuffle usage for now. (keep the shuffle rewrite, some of the logics are valid. just the modification to singleton exchange is not desired)
How about I change the default behavior to be disabled and enabled only requested? So we can use it when we think it is correct and understand the perf gain by 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
[GitHub] [pinot] siddharthteotia commented on pull request #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341649582
May want to update the naming as discussed offline to avoid confusion. Otherwise, this is good.
--
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] 61yao commented on pull request #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
61yao commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341656469
> May want to update the naming as discussed offline to avoid confusion. Otherwise, this is good.
Renamed the option. Thanks for the suggestion!
--
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] 61yao commented on pull request #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
61yao commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1340465794
> I am guessing we can use this to compare performance and even correctness between `broadcast join` , `shuffle join`, `colocated partitioning` table join ?
>
> I guess pretty much any scenario where we force the plan to not have an exchange ?
>
SkipShuffle is mostly used for doing colocated table join today. The main purpose is to compare colocated partitioning join and not.
> Another question - are we using `shuffle` and `exchange` terminology interchangeably ? IIRC, this was the case on the original design docs.
>
> So when we say `shuffle` is disabled, does that mean shuffle based exchange is disabled (but broadcast exchange can happen) or `exchange` all together is disabled ?
Disable is for disable SkipShuffle optimizer. SkipShuffle here means we are still doing exchange. but we are changing the broadcast exchange to singleton to favor local exchange. (we are already seeing problems caused by SkipShuffle and considering removing it all together )
--
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 #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1340400526
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9930?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9930](https://codecov.io/gh/apache/pinot/pull/9930?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7923199) into [master](https://codecov.io/gh/apache/pinot/commit/173916e42faee24dc1bb3f3d4de2c30bf2efd7a5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (173916e) will **decrease** coverage by `54.56%`.
> The diff coverage is `50.00%`.
```diff
@@ Coverage Diff @@
## master #9930 +/- ##
=============================================
- Coverage 70.46% 15.90% -54.57%
+ Complexity 5535 175 -5360
=============================================
Files 1982 1928 -54
Lines 106449 104022 -2427
Branches 16131 15843 -288
=============================================
- Hits 75006 16541 -58465
- Misses 26213 86255 +60042
+ Partials 5230 1226 -4004
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `15.90% <50.00%> (+0.07%)` | :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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9930?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/pinot/core/util/QueryOptionsUtils.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL1F1ZXJ5T3B0aW9uc1V0aWxzLmphdmE=) | `0.00% <0.00%> (-70.84%)` | :arrow_down: |
| [...va/org/apache/pinot/spi/utils/CommonConstants.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQ29tbW9uQ29uc3RhbnRzLmphdmE=) | `0.00% <ø> (-22.79%)` | :arrow_down: |
| [...ache/pinot/query/planner/logical/StagePlanner.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcGxhbm5lci9sb2dpY2FsL1N0YWdlUGxhbm5lci5qYXZh) | `97.50% <50.00%> (-2.50%)` | :arrow_down: |
| [...org/apache/pinot/query/context/PlannerContext.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcXVlcnktcGxhbm5lci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvY29udGV4dC9QbGFubmVyQ29udGV4dC5qYXZh) | `92.85% <100.00%> (+0.54%)` | :arrow_up: |
| [...src/main/java/org/apache/pinot/sql/FilterKind.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcWwvRmlsdGVyS2luZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...n/java/org/apache/pinot/core/data/table/Table.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1482 more](https://codecov.io/gh/apache/pinot/pull/9930/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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=The+Apache+Software+Foundation)
--
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] 61yao commented on pull request #9930: [multistage] [feature] Disable shuffle rewrite
Posted by GitBox <gi...@apache.org>.
61yao commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341771066
> > > let's remove all skipShuffle usage for now. (keep the shuffle rewrite, some of the logics are valid. just the modification to singleton exchange is not desired)
> >
> >
> > How about I change the default behavior to be disabled and enabled only requested? So we can use it when we think it is correct and understand the perf gain by this?
>
> yeah disabling the shuffle rewrite is good enough for now. no need to create an option for turning this on and off. for 2 reasons
>
> 1. we don't know how this is going to be used once we incorporated worker assignment strategy plugin
> 2. turning the current skipshuffle only works on a single 2 table join followed by a group by with the same key on the left table, which is severely limited.
> 3. creating a option here causes backward compatibility issues esp. we are close to a 0.12 release cut.
I don't feel we have much backward compatibility issue since it doesn't change default behavior.
Anyway, I updated the PR to disable it all together.
@ankitsultana FYK
--
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 #9930: [multistage] [bugfix] Disable shuffle rewrite
Posted by GitBox <gi...@apache.org>.
walterddr merged PR #9930:
URL: https://github.com/apache/pinot/pull/9930
--
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] siddharthteotia commented on pull request #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341628581
> > I am guessing we can use this to compare performance and even correctness between `broadcast join` , `shuffle join`, `colocated partitioning` table join ?
> > I guess pretty much any scenario where we force the plan to not have an exchange ?
>
> SkipShuffle is mostly used for doing colocated table join today. The main purpose is to compare colocated partitioning join and not.
>
> > Another question - are we using `shuffle` and `exchange` terminology interchangeably ? IIRC, this was the case on the original design docs.
> > So when we say `shuffle` is disabled, does that mean shuffle based exchange is disabled (but broadcast exchange can happen) or `exchange` all together is disabled ?
>
> Disable is for disable SkipShuffle optimizer. SkipShuffle here means we are still doing exchange. but we are changing the broadcast exchange to singleton to favor local exchange. (we are already seeing problems caused by SkipShuffle and considering removing it all together )
Got it !! Makes sense
--
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 #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341708487
hmm why is this necessary? i thought we established the fact that shuffle rewrite should not be used going forward.
--
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 #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341725087
let's remove all skipShuffle usage for now. (keep the shuffle rewrite, some of the logics are valid. just the modification to singleton hash is not desired)
--
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 #9930: [multistage] [bugfix] Disable shuffle rewrite
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9930:
URL: https://github.com/apache/pinot/pull/9930#discussion_r1042802928
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/StagePlanner.java:
##########
@@ -65,8 +65,6 @@ public QueryPlan makePlan(RelRoot relRoot) {
// walk the plan and create stages.
StageNode globalStageRoot = walkRelPlan(relRootNode, getNewStageId());
- ShuffleRewriteVisitor.optimizeShuffles(globalStageRoot);
-
Review Comment:
with this you will also have to disable the shuffle rewrite test in planner
--
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 #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
walterddr commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341747425
> > let's remove all skipShuffle usage for now. (keep the shuffle rewrite, some of the logics are valid. just the modification to singleton exchange is not desired)
>
> How about I change the default behavior to be disabled and enabled only requested? So we can use it when we think it is correct and understand the perf gain by this?
yeah disabling the shuffle rewrite is good enough for now. no need to create an option for turning this on and off. for 2 reasons
1. we don't know how this is going to be used once we incorporated worker assignment strategy plugin
2. turning the current skipshuffle only works on a single 2 table join followed by a group by with the same key on the left table, which is severely limited.
3. creating a option here causes backward compatibility issues esp. we are close to a 0.12 release cut.
--
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] 61yao commented on pull request #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
61yao commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1341721493
> hmm why is this necessary? i thought we established the fact that shuffle rewrite should not be used going forward.
Because we don't want to wait till that API change to happen.especially we are doing a bunch of perf testing now. it would be nice to have an option to disable this since we know it is problematic. Or I can remove all skipShuffle code now 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: 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] siddharthteotia commented on pull request #9930: [multistage] [feature] Add a query option to disable skip shuffle if needed
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on PR #9930:
URL: https://github.com/apache/pinot/pull/9930#issuecomment-1340387867
I am guessing we can use this to compare performance and even correctness between `broadcast join` , `shuffle join`, `colocated partitioning` table join ?
I guess pretty much any scenario where we force the plan to not have an exchange ?
Another question - are we using `shuffle` and `exchange` terminology interchangeably ? IIRC, this was the case on the original design docs.
So when we say `shuffle` is disabled, does that mean shuffle based exchange is disabled (but broadcast exchange can happen) or `exchange` all together is disabled ?
--
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