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/11/08 05:27:38 UTC
[PR] [multistage][bugfix] fix operator eos pull [pinot]
walterddr opened a new pull request, #11970:
URL: https://github.com/apache/pinot/pull/11970
currently agg, window, transform, fitler operator can potentially pull more than once against input for EOS.
This PR changes the behavior.
- single-input stop-the-world operator (agg, window, sort) only pull once and check for produced block first
- streaming operator (filter, transform) keeps the behavior
- dual-input, right-sided stop-the-world & left-sided stream operator (join, set) will only pull EOS on each input at most once
- when right side error out first, left side will not be pulled
this guarantees that no more than one EOS block will be pulled
--
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] [multistage][bugfix] fix operator eos pull [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11970:
URL: https://github.com/apache/pinot/pull/11970#discussion_r1387375479
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/MultiStageOperator.java:
##########
@@ -70,7 +79,7 @@ public String getOperatorId() {
}
// Make it protected because we should always call nextBlock()
- protected abstract TransferableBlock getNextBlock();
+ protected abstract TransferableBlock getNextBlock() throws Exception;
Review Comment:
(minor) Reformat
--
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] [multistage][bugfix] fix operator eos pull [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11970:
URL: https://github.com/apache/pinot/pull/11970#discussion_r1386822846
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SetOperator.java:
##########
@@ -89,12 +92,23 @@ public ExecutionStatistics getExecutionStatistics() {
@Override
protected TransferableBlock getNextBlock() {
- // A blocking call to construct a set with all the right side rows.
- if (!_isRightSetBuilt) {
- constructRightBlockSet();
+ try {
+ if (_isTerminated) {
+ return TransferableBlockUtils.getEndOfStreamTransferableBlock();
+ }
+ if (!_isRightSetBuilt) {
+ // construct a SET with all the right side rows.
+ constructRightBlockSet();
+ }
+ if (_upstreamErrorBlock != null) {
Review Comment:
yeah we should've :-P the issue here is similar to JOIN, fixed
--
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] [multistage][bugfix] fix operator eos pull [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11970:
URL: https://github.com/apache/pinot/pull/11970#issuecomment-1801145513
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11970](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b06ac0a) into [master](https://app.codecov.io/gh/apache/pinot/commit/45f186903e0d35304fb764ccb5f1423839d3276b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (45f1869) will **decrease** coverage by `14.98%`.
> Report is 3 commits behind head on master.
> The diff coverage is `85.48%`.
```diff
@@ Coverage Diff @@
## master #11970 +/- ##
=============================================
- Coverage 61.43% 46.46% -14.98%
+ Complexity 1135 927 -208
=============================================
Files 2385 1787 -598
Lines 129150 93574 -35576
Branches 19994 15135 -4859
=============================================
- Hits 79347 43478 -35869
- Misses 44049 47031 +2982
+ Partials 5754 3065 -2689
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/11970/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/11970/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/11970/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/11970/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/11970/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/11970/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/11970/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <85.48%> (-14.86%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/11970/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/11970/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <85.48%> (-14.84%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/11970/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <85.48%> (-14.98%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/11970/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <85.48%> (-14.97%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/11970/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.46% <85.48%> (-0.21%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/11970/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.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...inot/query/runtime/operator/AggregateOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9BZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | `84.80% <100.00%> (ø)` | |
| [...pinot/query/runtime/operator/HashJoinOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9IYXNoSm9pbk9wZXJhdG9yLmphdmE=) | `94.01% <ø> (ø)` | |
| [...he/pinot/query/runtime/operator/OperatorStats.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9PcGVyYXRvclN0YXRzLmphdmE=) | `93.02% <ø> (-0.32%)` | :arrow_down: |
| [...inot/query/runtime/operator/TransformOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9UcmFuc2Zvcm1PcGVyYXRvci5qYXZh) | `100.00% <100.00%> (ø)` | |
| [...uery/runtime/operator/WindowAggregateOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9XaW5kb3dBZ2dyZWdhdGVPcGVyYXRvci5qYXZh) | `95.97% <100.00%> (-0.03%)` | :arrow_down: |
| [...che/pinot/query/runtime/operator/SortOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9Tb3J0T3BlcmF0b3IuamF2YQ==) | `93.33% <97.36%> (-0.42%)` | :arrow_down: |
| [...e/pinot/query/runtime/operator/FilterOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9GaWx0ZXJPcGVyYXRvci5qYXZh) | `90.90% <50.00%> (-9.10%)` | :arrow_down: |
| [...ache/pinot/query/runtime/operator/SetOperator.java](https://app.codecov.io/gh/apache/pinot/pull/11970?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtcXVlcnktcnVudGltZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvcXVlcnkvcnVudGltZS9vcGVyYXRvci9TZXRPcGVyYXRvci5qYXZh) | `65.21% <50.00%> (-8.47%)` | :arrow_down: |
... and [983 files with indirect coverage changes](https://app.codecov.io/gh/apache/pinot/pull/11970/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in [Chrome](https://chrome.google.com/webstore/detail/codecov/gedikamndpbemklijjkncpnolildpbgo) or [Firefox](https://addons.mozilla.org/en-US/firefox/addon/codecov/) today!
--
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] [multistage][bugfix] fix operator eos pull [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11970:
URL: https://github.com/apache/pinot/pull/11970#discussion_r1386136995
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SetOperator.java:
##########
@@ -89,12 +92,23 @@ public ExecutionStatistics getExecutionStatistics() {
@Override
protected TransferableBlock getNextBlock() {
- // A blocking call to construct a set with all the right side rows.
- if (!_isRightSetBuilt) {
- constructRightBlockSet();
+ try {
+ if (_isTerminated) {
+ return TransferableBlockUtils.getEndOfStreamTransferableBlock();
+ }
+ if (!_isRightSetBuilt) {
+ // construct a SET with all the right side rows.
+ constructRightBlockSet();
+ }
+ if (_upstreamErrorBlock != null) {
Review Comment:
We probably don't need to save this as member variable. Also, the error from right table is not gathered
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/AggregateOperator.java:
##########
@@ -132,25 +132,22 @@ public String toExplainString() {
@Override
protected TransferableBlock getNextBlock() {
try {
Review Comment:
Do we always need to do this try-catch? Consider moving it into base class?
--
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] [multistage][bugfix] fix operator eos pull [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #11970:
URL: https://github.com/apache/pinot/pull/11970#discussion_r1386822755
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/AggregateOperator.java:
##########
@@ -132,25 +132,22 @@ public String toExplainString() {
@Override
protected TransferableBlock getNextBlock() {
try {
Review Comment:
yes i can move it to base
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/SetOperator.java:
##########
@@ -89,12 +92,23 @@ public ExecutionStatistics getExecutionStatistics() {
@Override
protected TransferableBlock getNextBlock() {
- // A blocking call to construct a set with all the right side rows.
- if (!_isRightSetBuilt) {
- constructRightBlockSet();
+ try {
+ if (_isTerminated) {
+ return TransferableBlockUtils.getEndOfStreamTransferableBlock();
+ }
+ if (!_isRightSetBuilt) {
+ // construct a SET with all the right side rows.
+ constructRightBlockSet();
+ }
+ if (_upstreamErrorBlock != null) {
Review Comment:
yeah we should've :-P the issue here is similar to JOIN, fixing
--
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] [multistage][bugfix] fix operator eos pull [pinot]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr merged PR #11970:
URL: https://github.com/apache/pinot/pull/11970
--
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