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/08/26 02:33:24 UTC
[GitHub] [pinot] Jackie-Jiang opened a new pull request, #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks
Jackie-Jiang opened a new pull request, #9282:
URL: https://github.com/apache/pinot/pull/9282
Currently `IntermediateResultsBlock` is used for all types of queries, which makes it quite complicated because it needs to hold all different types of responses and handle the data table conversion for all of them.
This PR breaks it into different results blocks for different types of queries so that the logic is simpler, and future changes are easier
--
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] Jackie-Jiang commented on a diff in pull request #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9282:
URL: https://github.com/apache/pinot/pull/9282#discussion_r957812873
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/AcquireReleaseColumnsSegmentOperator.java:
##########
@@ -58,8 +58,8 @@ public AcquireReleaseColumnsSegmentOperator(PlanNode planNode, IndexSegment inde
* Runs the planNode to get the childOperator, and then proceeds with execution.
*/
@Override
- protected IntermediateResultsBlock getNextBlock() {
- _childOperator = (Operator<IntermediateResultsBlock>) _planNode.run();
+ protected BaseResultsBlock getNextBlock() {
+ _childOperator = (Operator<BaseResultsBlock>) _planNode.run();
return _childOperator.nextBlock();
Review Comment:
This is a workaround to acquire the segment before running the plan. Currently the plan is physical execution plan, which is per-segment based.
--
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] Jackie-Jiang commented on a diff in pull request #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9282:
URL: https://github.com/apache/pinot/pull/9282#discussion_r957828740
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -273,14 +273,13 @@ public Plan makeStreamingInstancePlan(List<IndexSegment> indexSegments, QueryCon
if (QueryContextUtils.isSelectionOnlyQuery(queryContext)) {
// selection-only is streamed in StreamingSelectionPlanNode --> here only metadata block is returned.
return new GlobalPlanImplV0(
- new InstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext,
- serverMetrics));
+ new InstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext));
} else {
// non-selection-only requires a StreamingInstanceResponsePlanNode to stream data block back and metadata block
// as final return.
return new GlobalPlanImplV0(
- new StreamingInstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(),
- streamObserver, queryContext, serverMetrics));
+ new StreamingInstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext,
+ streamObserver));
Review Comment:
Trying to keep it closer to the non-streaming one :-P
--
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 merged pull request #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks
Posted by GitBox <gi...@apache.org>.
siddharthteotia merged PR #9282:
URL: https://github.com/apache/pinot/pull/9282
--
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 #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks
Posted by GitBox <gi...@apache.org>.
walterddr commented on code in PR #9282:
URL: https://github.com/apache/pinot/pull/9282#discussion_r957726100
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -273,14 +273,13 @@ public Plan makeStreamingInstancePlan(List<IndexSegment> indexSegments, QueryCon
if (QueryContextUtils.isSelectionOnlyQuery(queryContext)) {
// selection-only is streamed in StreamingSelectionPlanNode --> here only metadata block is returned.
return new GlobalPlanImplV0(
- new InstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext,
- serverMetrics));
+ new InstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext));
} else {
// non-selection-only requires a StreamingInstanceResponsePlanNode to stream data block back and metadata block
// as final return.
return new GlobalPlanImplV0(
- new StreamingInstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(),
- streamObserver, queryContext, serverMetrics));
+ new StreamingInstanceResponsePlanNode(combinePlanNode, indexSegments, Collections.emptyList(), queryContext,
+ streamObserver));
Review Comment:
why are we switching the order of the stream observer and query context?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java:
##########
@@ -34,24 +34,21 @@
public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
-
private static final String EXPLAIN_NAME = "INSTANCE_RESPONSE";
- private final BaseCombineOperator _combineOperator;
+ private final BaseCombineOperator<?> _combineOperator;
Review Comment:
should we also put type generic here for InstanceResponseOperator<T extends BaseResultBlock> ?
eventually resultsBlock.getDataTable(QueryContext) is implemented at each individual `T` correct?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java:
##########
@@ -34,24 +34,21 @@
public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
-
private static final String EXPLAIN_NAME = "INSTANCE_RESPONSE";
- private final BaseCombineOperator _combineOperator;
+ private final BaseCombineOperator<?> _combineOperator;
private final List<IndexSegment> _indexSegments;
private final List<FetchContext> _fetchContexts;
private final int _fetchContextSize;
private final QueryContext _queryContext;
- private final ServerMetrics _serverMetrics;
- public InstanceResponseOperator(BaseCombineOperator combinedOperator, List<IndexSegment> indexSegments,
- List<FetchContext> fetchContexts, QueryContext queryContext, ServerMetrics serverMetrics) {
+ public InstanceResponseOperator(BaseCombineOperator<?> combinedOperator, List<IndexSegment> indexSegments,
+ List<FetchContext> fetchContexts, QueryContext queryContext) {
_combineOperator = combinedOperator;
Review Comment:
nit: this and all others, `combinedOperator` -> `combineOperator`
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/AcquireReleaseColumnsSegmentOperator.java:
##########
@@ -58,8 +58,8 @@ public AcquireReleaseColumnsSegmentOperator(PlanNode planNode, IndexSegment inde
* Runs the planNode to get the childOperator, and then proceeds with execution.
*/
@Override
- protected IntermediateResultsBlock getNextBlock() {
- _childOperator = (Operator<IntermediateResultsBlock>) _planNode.run();
+ protected BaseResultsBlock getNextBlock() {
+ _childOperator = (Operator<BaseResultsBlock>) _planNode.run();
return _childOperator.nextBlock();
Review Comment:
unrelated to this PR but why are we running the plannode everytime to get child operator? i know it will access segment before running the plan, but does it change on a per segment bases?
--
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 #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9282:
URL: https://github.com/apache/pinot/pull/9282#issuecomment-1228031116
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9282?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 [#9282](https://codecov.io/gh/apache/pinot/pull/9282?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (806a07f) into [master](https://codecov.io/gh/apache/pinot/commit/88bb40c3467033f8e2693526c2d2111117f446f4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (88bb40c) will **increase** coverage by `1.50%`.
> The diff coverage is `89.85%`.
```diff
@@ Coverage Diff @@
## master #9282 +/- ##
============================================
+ Coverage 68.42% 69.93% +1.50%
+ Complexity 5016 4695 -321
============================================
Files 1864 1870 +6
Lines 99469 99436 -33
Branches 15133 15121 -12
============================================
+ Hits 68060 69537 +1477
+ Misses 26560 24968 -1592
- Partials 4849 4931 +82
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `26.26% <65.07%> (?)` | |
| integration2 | `24.90% <70.74%> (+0.06%)` | :arrow_up: |
| unittests1 | `67.10% <83.58%> (+0.01%)` | :arrow_up: |
| unittests2 | `15.29% <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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9282?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...operator/AcquireReleaseColumnsSegmentOperator.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9BY3F1aXJlUmVsZWFzZUNvbHVtbnNTZWdtZW50T3BlcmF0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ot/core/operator/blocks/InstanceResponseBlock.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9ibG9ja3MvSW5zdGFuY2VSZXNwb25zZUJsb2NrLmphdmE=) | `50.00% <ø> (-6.25%)` | :arrow_down: |
| [...ot/core/operator/combine/CombineOperatorUtils.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9jb21iaW5lL0NvbWJpbmVPcGVyYXRvclV0aWxzLmphdmE=) | `100.00% <ø> (ø)` | |
| [...a/org/apache/pinot/core/plan/DistinctPlanNode.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL0Rpc3RpbmN0UGxhbk5vZGUuamF2YQ==) | `86.95% <ø> (ø)` | |
| [.../org/apache/pinot/core/plan/SelectionPlanNode.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9wbGFuL1NlbGVjdGlvblBsYW5Ob2RlLmphdmE=) | `100.00% <ø> (ø)` | |
| [...perator/query/DictionaryBasedDistinctOperator.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9EaWN0aW9uYXJ5QmFzZWREaXN0aW5jdE9wZXJhdG9yLmphdmE=) | `76.08% <50.00%> (ø)` | |
| [.../pinot/core/operator/InstanceResponseOperator.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9JbnN0YW5jZVJlc3BvbnNlT3BlcmF0b3IuamF2YQ==) | `75.00% <72.72%> (-2.36%)` | :arrow_down: |
| [.../core/operator/query/SelectionOrderByOperator.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9xdWVyeS9TZWxlY3Rpb25PcmRlckJ5T3BlcmF0b3IuamF2YQ==) | `87.84% <75.00%> (ø)` | |
| [...reaming/StreamingSelectionOnlyCombineOperator.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9zdHJlYW1pbmcvU3RyZWFtaW5nU2VsZWN0aW9uT25seUNvbWJpbmVPcGVyYXRvci5qYXZh) | `66.00% <84.61%> (ø)` | |
| [...e/operator/blocks/results/GroupByResultsBlock.java](https://codecov.io/gh/apache/pinot/pull/9282/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-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9ibG9ja3MvcmVzdWx0cy9Hcm91cEJ5UmVzdWx0c0Jsb2NrLmphdmE=) | `85.71% <85.71%> (ø)` | |
| ... and [165 more](https://codecov.io/gh/apache/pinot/pull/9282/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] Jackie-Jiang commented on a diff in pull request #9282: [Cleanup] Break IntermediateResultsBlock into different results blocks
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on code in PR #9282:
URL: https://github.com/apache/pinot/pull/9282#discussion_r957827047
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/InstanceResponseOperator.java:
##########
@@ -34,24 +34,21 @@
public class InstanceResponseOperator extends BaseOperator<InstanceResponseBlock> {
-
private static final String EXPLAIN_NAME = "INSTANCE_RESPONSE";
- private final BaseCombineOperator _combineOperator;
+ private final BaseCombineOperator<?> _combineOperator;
Review Comment:
We only need that in the combine operator because we need to know the `T` when merging the blocks. the `getDataTable()` is a common interface, so we don't need to know the exact type
--
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