You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by "xiaokang (via GitHub)" <gi...@apache.org> on 2024/02/21 04:32:18 UTC
[PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
xiaokang opened a new pull request, #31199:
URL: https://github.com/apache/doris/pull/31199
## Proposed changes
Issue Number: close #xxx
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31199:
URL: https://github.com/apache/doris/pull/31199#issuecomment-1955947341
PR approved by anyone and no changes requested.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #31199:
URL: https://github.com/apache/doris/pull/31199#issuecomment-1955889682
TeamCity be ut coverage result:
Function Coverage: 35.72% (8558/23959)
Line Coverage: 27.61% (69456/251563)
Region Coverage: 26.74% (36033/134751)
Branch Coverage: 23.56% (18431/78240)
Coverage Report: http://coverage.selectdb-in.cc/coverage/caf44a25c06235355dd9a0be22b25e137245e5a7_caf44a25c06235355dd9a0be22b25e137245e5a7/report/index.html
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #31199:
URL: https://github.com/apache/doris/pull/31199#issuecomment-1955872770
Thank you for your contribution to Apache Doris.
Don't know what should be done next? See [How to process your PR](https://cwiki.apache.org/confluence/display/DORIS/How+to+process+your+PR)
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31199:
URL: https://github.com/apache/doris/pull/31199#issuecomment-1955947310
PR approved by at least one committer and no changes requested.
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #31199:
URL: https://github.com/apache/doris/pull/31199#discussion_r1496894202
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -2062,7 +2062,30 @@ Status SegmentIterator::_read_columns_by_rowids(std::vector<ColumnId>& read_colu
}
Status SegmentIterator::next_batch(vectorized::Block* block) {
- auto status = [&]() { RETURN_IF_CATCH_EXCEPTION({ return _next_batch_internal(block); }); }();
+ auto status = [&]() {
+ RETURN_IF_CATCH_EXCEPTION({
+ RETURN_IF_ERROR(_next_batch_internal(block));
+
+ // reverse block row order if read_orderby_key_reverse is true for key topn
+ // it should be processed for all success _next_batch_internal
+ if (_opts.read_orderby_key_reverse) {
+ size_t num_rows = block->rows();
+ if (num_rows == 0) {
+ return Status::OK();
+ }
+ size_t num_columns = block->columns();
+ vectorized::IColumn::Permutation permutation;
+ for (size_t i = 0; i < num_rows; ++i) permutation.emplace_back(num_rows - 1 - i);
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
for (size_t i = 0; i < num_rows; ++i) { permutation.emplace_back(num_rows - 1 - i);
}
```
##########
be/src/olap/rowset/segment_v2/segment_iterator.cpp:
##########
@@ -2062,7 +2062,30 @@
}
Status SegmentIterator::next_batch(vectorized::Block* block) {
- auto status = [&]() { RETURN_IF_CATCH_EXCEPTION({ return _next_batch_internal(block); }); }();
+ auto status = [&]() {
+ RETURN_IF_CATCH_EXCEPTION({
+ RETURN_IF_ERROR(_next_batch_internal(block));
+
+ // reverse block row order if read_orderby_key_reverse is true for key topn
+ // it should be processed for all success _next_batch_internal
+ if (_opts.read_orderby_key_reverse) {
+ size_t num_rows = block->rows();
+ if (num_rows == 0) {
+ return Status::OK();
+ }
+ size_t num_columns = block->columns();
+ vectorized::IColumn::Permutation permutation;
+ for (size_t i = 0; i < num_rows; ++i) permutation.emplace_back(num_rows - 1 - i);
+
+ for (size_t i = 0; i < num_columns; ++i)
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
for (size_t i = 0; i < num_columns; ++i) {
```
be/src/olap/rowset/segment_v2/segment_iterator.cpp:2081:
```diff
- block->get_by_position(i).column->permute(permutation, num_rows);
+ block->get_by_position(i).column->permute(permutation, num_rows);
+ }
```
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "doris-robot (via GitHub)" <gi...@apache.org>.
doris-robot commented on PR #31199:
URL: https://github.com/apache/doris/pull/31199#issuecomment-1955888252
<details>
<summary>TPC-H: <b>Total hot run time: 41374 ms</b></summary>
```
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit caf44a25c06235355dd9a0be22b25e137245e5a7, data reload: false
------ Round 1 ----------------------------------
q1 17632 4965 4839 4839
q2 2054 145 137 137
q3 10673 995 998 995
q4 4758 964 960 960
q5 7725 3244 3257 3244
q6 195 137 133 133
q7 1248 784 757 757
q8 9618 2062 2065 2062
q9 7954 6695 6662 6662
q10 8355 2629 2626 2626
q11 420 222 219 219
q12 1329 329 336 329
q13 18058 3662 3651 3651
q14 295 257 261 257
q15 589 518 531 518
q16 498 415 419 415
q17 939 884 874 874
q18 7196 6949 6606 6606
q19 1539 1480 1479 1479
q20 563 350 344 344
q21 5837 3976 3933 3933
q22 397 335 334 334
Total cold run time: 107872 ms
Total hot run time: 41374 ms
----- Round 2, with runtime_filter_mode=off -----
q1 4850 4800 4797 4797
q2 288 186 180 180
q3 3574 3554 3553 3553
q4 2486 2499 2511 2499
q5 5743 5699 5737 5699
q6 211 126 129 126
q7 2227 1635 1642 1635
q8 3009 3099 3107 3099
q9 8659 8688 8691 8688
q10 4118 4203 4226 4203
q11 527 376 365 365
q12 756 525 538 525
q13 4179 3383 3361 3361
q14 274 225 227 225
q15 639 498 499 498
q16 488 424 465 424
q17 1683 1600 1586 1586
q18 8351 7591 7584 7584
q19 1631 1635 1644 1635
q20 2107 1835 1816 1816
q21 6503 6101 6078 6078
q22 557 494 494 494
Total cold run time: 62860 ms
Total hot run time: 59070 ms
```
</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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "yiguolei (via GitHub)" <gi...@apache.org>.
yiguolei merged PR #31199:
URL: https://github.com/apache/doris/pull/31199
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
Re: [PR] [fix](topn) Fix key topn block reverse is missed in some cases [doris]
Posted by "xiaokang (via GitHub)" <gi...@apache.org>.
xiaokang commented on PR #31199:
URL: https://github.com/apache/doris/pull/31199#issuecomment-1955872862
run buildall
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org