[refactor](be) Remove scanner eos flag#63578
Open
mrhhsg wants to merge 1 commit into
Open
Conversation
Contributor
|
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: The scan projection path used a misspelled `_alreay_eos` boolean only to remember that a final padding block had to be emitted after get_block() reported eos. The extra state is not needed: when get_block() reports eos, the projection path can merge the pending padding block and the final origin block directly, then return eos in the same call. This may make the final output block exceed the target batch size, but the lower scanner already bounds individual source blocks, and avoiding a delayed eos handoff keeps the state machine simpler. A targeted unit test covers the limit-eos case where both origin and padding blocks are pending.
### Release note
None
### Check List (For Author)
- Test: Unit Test / Compile check
- Formatted modified C++ files with build-support/run_clang_format.py using clang-format 16.
- `git diff --check`
- `ninja -C be/ut_build_ASAN -j 1 src/exec/CMakeFiles/Exec.dir/scan/scanner.cpp.o test/CMakeFiles/doris_be_test.dir/exec/scan/scanner_late_arrival_rf_test.cpp.o`
- Attempted `./run-be-ut.sh --run -j 1 --filter=ScannerProjectionTest.merges_padding_block_when_limit_eos_without_extra_flag:ScannerLateArrivalRfTest.applied_rf_num_advances_after_late_arrival`, but the full BE UT target did not complete locally because it started a broad rebuild on the shared host and process-resource pressure was observed (`fork: Resource temporarily unavailable`).
- Behavior changed: No query result behavior changed. Internal scanner block boundaries can change for the final eos block.
- Does this need documentation: No
mrhhsg
force-pushed
the
codex/remove-scanner-alreay-eos
branch
from
492ee1f to
dad2510
Compare
Member
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 completed. I found no blocking issues in the actual PR diff (scanner.cpp, scanner.h, and scanner_late_arrival_rf_test.cpp).
Critical checkpoint conclusions:
- Goal and tests: The PR removes the delayed
_alreay_eosstate and returns the merged final projection block witheos=true; the new unit test covers the limit-EOS case with both padding and origin blocks pending. - Scope: The change is small and focused on the scanner projection path plus a targeted test.
- Concurrency: No new shared state, locks, or thread handoff are introduced. The existing scheduler path can enqueue a non-empty block while marking the scanner EOS.
- Lifecycle/static state: No new lifecycle-sensitive objects or static initialization dependencies.
- Configuration/compatibility/protocol: No config, storage format, wire protocol, or compatibility changes.
- Parallel paths: The non-projection path is unchanged; the changed path is specific to
_output_row_descriptorprojection handling. - Error handling: Existing
Statuspropagation fromget_block,_merge_padding_block, and projection execution is preserved. - Test coverage/results: Added focused BE unit coverage. I did not rerun tests during this automated review.
- Observability/transactions/data writes: Not applicable; no new write path or observability requirement.
- Performance: Final EOS block boundaries may be larger as described, but source blocks remain bounded and this removes an extra scanner state transition.
User focus points: No additional user-provided review focus was present.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31756 ms |
Contributor
TPC-DS: Total hot run time: 171297 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
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 changed
Remove the extra
_alreay_eosscanner state from the scan projection path.When
get_block()reports eos while both_padding_blockand the final_origin_blockcontain data, the projection path now merges them directly and returnseos=truein the same call. This can make only the final output block larger than the normal batch target, but each source block is already bounded by the lower scanner.Why
_alreay_eosonly existed to carry the final eos handoff across calls, and it also carried a typo in the member name. Merging the final padding/origin blocks at eos removes that extra state and keeps the block lifecycle simpler without changing query results.Validation
build-support/run_clang_format.pyusing clang-format 16.git diff --checkninja -C be/ut_build_ASAN -j 1 src/exec/CMakeFiles/Exec.dir/scan/scanner.cpp.o test/CMakeFiles/doris_be_test.dir/exec/scan/scanner_late_arrival_rf_test.cpp.o./run-be-ut.sh --run -j 1 --filter=ScannerProjectionTest.merges_padding_block_when_limit_eos_without_extra_flag:ScannerLateArrivalRfTest.applied_rf_num_advances_after_late_arrival; the full BE UT target did not complete locally because it started a broad rebuild on the shared host and process-resource pressure was observed (fork: Resource temporarily unavailable).Release note
None