[refine](sort) Remove heap sorter runtime predicate guard#63937
Closed
Mryange wants to merge 1 commit into
Closed
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: no blocking findings found in the changed heap-sorter runtime-predicate path.
Critical checkpoint conclusions:
- Goal and proof: The PR removes the runtime-predicate guard so HeapSorter always uses its local TopN filtering once the heap is full. The updated unit test covers the changed block-mutation expectation, though I did not run the BE unit test in this runner.
- Scope: The change is small and focused on HeapSorter construction and the append fast path.
- Concurrency: No new shared state or locking is introduced. Existing runtime predicate update remains in SortSinkOperatorX::sink after append_block.
- Lifecycle/static state: No new lifecycle management or static/global initialization concerns.
- Configuration/compatibility: No config, protocol, storage, or serialization compatibility changes.
- Parallel paths: TOPN_SORT and FULL_SORT paths are unchanged; the change is specific to HEAP_SORT, which is where the runtime predicate source is handled.
- Tests: Existing heap sorter unit test was adjusted; no regression or unit test execution was performed locally.
- Observability: Existing SortSink counters/timers remain, including TopNFilterTime/TopNFilterRows and UpdateRuntimePredicateTime.
- Data correctness: Local heap filtering and runtime predicate updates both use the current heap top bound; I did not find a path where enabling both drops rows that should remain in the final top-N.
- Performance: Removing the guard enables the existing local filter in more cases and removes one QueryContext lookup during sorter construction.
User focus: No additional user-provided review focus was present.
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run external |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29484 ms |
Contributor
TPC-DS: Total hot run time: 171010 ms |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
HeapSorter kept a
have_runtime_predicateflag and skipped its local TopN filtering path whenever the sort node had a runtime predicate. The heap sorter should no longer branch on whether a runtime predicate exists. This PR removes that constructor argument and member state, and always uses the existing heap filter path once the heap queue is valid and has reached the heap size.The sort sink no longer queries
QueryContext::has_runtime_predicate()when constructingHeapSorter. The heap sorter unit test is also updated so it checks the expected top value directly instead of reading it from an input block that may be modified by the filtering path.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)