Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: BlockSupplierSortCursorImpl requires every supplied block to
contain rows unless EOF is set. The existing DCHECK disappears in release
builds, allowing execution to continue if this contract is violated. Replace
it with DORIS_CHECK so release builds throw through the existing exception
handling instead of allowing an invalid cursor to reach the merge heap.
The check runs per input block, not per row. This is an invariant enforcement
change; the reported production crash has not been reproduced or attributed
to an empty input block.
### Release note
Sorted merge queries report an error in release builds if a block supplier
returns an empty block without EOF. Debug builds retain fatal checking.
### Check List (For Author)
- Test: Manual checks
- Passed clang-format 16.0.5 via check-format.sh on an isolated copy of the changed file
- Passed build-support/check-build-hygiene.sh and git diff --check
- Attempted ./run-be-ut.sh --run --filter='SortMergerTest.*'; stopped during Java environment validation because no Java Runtime is available
- BE build and clang-tidy not run; no compilation database is available
- Behavior changed: Yes; enforce the existing input invariant in release builds
- Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 27564 ms |
Contributor
TPC-DS: Total hot run time: 153443 ms |
Contributor
ClickBench: Total hot run time: 23.74 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Mryange
approved these changes
Sep 24, 2026
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review conclusion: request changes — one blocking exception-boundary issue.
Critical checkpoints:
- Goal and correctness: the one-line change is focused and enforces the supplier invariant on ordinary release paths, but it does not universally turn violations into query errors. A later sorted-Iceberg spill refill can throw outside any catch and terminate the BE.
- Parallel paths: exchange, local merge sort, and generic spill sort refills run beneath the pipeline scheduler's exception boundary; the asynchronous Iceberg writer close path does not. All current supplier registrations and both merger refill sites were traced.
- Concurrency and lifecycle: no new shared state, locking, ownership, or static-initialization issue was found. The blocking issue is exception lifecycle across the generic async-writer thread, which also bypasses writer-status publication and ready-to-finish signaling.
- Configuration, compatibility, persistence, and data contracts: no configuration, FE/BE protocol, function symbol, storage format, transaction, journal/replay, or rolling-upgrade change is introduced.
- Performance and observability: the success-path predicate runs once per supplied block and has negligible cost. Fatal status/stack information is adequate when caught, but the accepted path has no boundary that can publish it as a query status.
- Tests: existing merger tests cover valid ready/pending and EOF flows, and
DorisCheckTestcovers build-mode behavior, but there is no valid-first/empty-non-EOF refill test for the async caller. Exact-head CI reports compile and BE UT success; this review was static only and did not run builds or tests, as required by the review environment. - User focus: no additional focus points were supplied.
The single accepted issue is attached inline; no other substantiated or unresolved point remains after two converged review rounds.
| block->clear(); | ||
| THROW_IF_ERROR(_block_supplier(block.get(), &_is_eof)); | ||
| DCHECK(!block->empty() or _is_eof); | ||
| DORIS_CHECK(!block->empty() || _is_eof); |
Contributor
There was a problem hiding this comment.
[P1] Preserve a catch boundary for refill checks
DORIS_CHECK throws a doris::Exception in release builds. Initial cursor fetches are caught by VSortedRunMerger::prepare, and pipeline refill calls are caught by TaskScheduler, but sorted Iceberg partition close runs _combine_files_output() / _do_intermediate_merge() from AsyncResultWriter's generic thread-pool task. Those paths call VSortedRunMerger::get_next() for later refills without any catch, and neither the writer lambda nor ThreadPool catches exceptions. If this invariant fires there (for example, a valid first spill block followed by a malformed zero-length entry), the exception crosses the thread entry and invokes std::terminate, crashing the BE instead of reporting the query error described by this PR. Please catch and convert refill exceptions centrally in VSortedRunMerger::get_next() (covering both refill sites), and add a release-mode test with a valid first block followed by an empty non-EOF block.
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
3 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?
Issue Number: None
Related PR: None
Problem Summary: BlockSupplierSortCursorImpl requires every supplied block to contain rows unless EOF is set. The existing DCHECK disappears in release builds, allowing execution to continue if this contract is violated. Replace it with DORIS_CHECK so release builds throw through the existing exception handling instead of allowing an invalid cursor to reach the merge heap. The check runs per input block, not per row. This is an invariant enforcement change; the reported production crash has not been reproduced or attributed to an empty input block.
Release note
Sorted merge queries report an error in release builds if a block supplier returns an empty block without EOF. Debug builds retain fatal checking.
Check List (For Author)
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)