Uh oh!
There was an error while loading. Please reload this page.
[opt](sort) avoid copying whole blocks during merge - #63429
Conversation
hello-stephen
commented
May 20, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: MergeSorterState used the generic row-copy merge path even when the current top run could return its whole remaining block before any other run. This adds a direct whole-block path guarded by a total-order check. ### Release note None ### Check List (For Author) - Test: Unit Test - BE UT: sh run-be-ut.sh --run --filter='MergeSorterStateTest.*' - Behavior changed: No - Does this need documentation: No
ca5da84 to
a2e0238CompareMryange
commented
May 20, 2026
/review |
Mryange
commented
May 20, 2026
run buildall |
hello-stephen
commented
May 20, 2026
TPC-H: Total hot run time: 31977 ms |
hello-stephen
commented
May 20, 2026
TPC-DS: Total hot run time: 169378 ms |
Mryange
commented
May 20, 2026
run buildall |
dd5aa44 to
1e5bf1cCompareMryange
commented
May 20, 2026
run buildall |
hello-stephen
commented
May 20, 2026
TPC-H: Total hot run time: 31585 ms |
hello-stephen
commented
May 20, 2026
TPC-DS: Total hot run time: 171135 ms |
hello-stephen
commented
May 20, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
May 20, 2026
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
May 20, 2026
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
hello-stephen
commented
May 20, 2026
TPC-H: Total hot run time: 30985 ms |
hello-stephen
commented
May 20, 2026
TPC-DS: Total hot run time: 169153 ms |
Mryange
commented
May 21, 2026
/review |
There was a problem hiding this comment.
Review summary: no blocking issues found in the actual PR diff.
Critical checkpoint conclusions:
- Goal/test coverage: the PR adds a whole-block fast path for merge-sort reads and adds focused BE unit coverage for block swapping and smaller-than-batch behavior. Existing sort operator/heap sorter tests were adjusted to consume multiple output batches, which matches the new behavior.
- Scope: the actual GitHub PR contains only sort fast-path/test changes and is focused.
- Concurrency/lifecycle: no new shared state, threads, locks, or special lifecycle concerns were introduced.
- Configuration/compatibility/persistence: no new config items, storage formats, RPC/thrift/proto changes, or rolling-upgrade compatibility concerns in the actual PR diff.
- Parallel paths: FullSorter/HeapSorter/TopNSorter all use MergeSorterState, so the optimization is applied at the common merge-sort read path.
- Error handling/memory: no Status paths are discarded. The fast path transfers Block ownership by swap only when the current run can be returned whole, avoiding extra copies; it preserves the existing two-call EOS contract.
- Performance: the optimization is reasonable and avoids per-row/per-column copying when one sorted block is wholly before the next run.
- User focus: no additional user-provided review focus was present.
Residual risk: I did not run BE unit tests in this review environment; the conclusion is based on code inspection and the PR patch from GitHub.
Uh oh!
There was an error while loading. Please reload this page.
Problem Summary: `MergeSorterState` used the generic copy-based merge path even when the current top sorted run could return its whole remaining block before any other run. This adds a direct whole-block fast path guarded by a total-order check, avoiding unnecessary `insert_range_from` work in inner merge. ### What is changed? - Add `MergeSortCursor::totally_less_or_equals()` to detect when the current run is wholly before the next child. - Return the current block directly from `MergeSorterState::_merge_sort_read_impl()` when the whole-block condition is satisfied. - Add focused BE unit tests for exact-batch and smaller-than-batch whole-block fast-path cases.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
MergeSorterStateused the generic copy-based merge path even when the current top sorted run could return its whole remaining block before any other run. This adds a direct whole-block fast path guarded by a total-order check, avoiding unnecessaryinsert_range_fromwork in inner merge.What is changed?
MergeSortCursor::totally_less_or_equals()to detect when the current run is wholly before the next child.MergeSorterState::_merge_sort_read_impl()when the whole-block condition is satisfied.Release note
None