[refactor](be) Move adaptive block size predictor to core - #65995
Merged
mrhhsg merged 1 commit intoJul 31, 2026
Conversation
mrhhsg
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
July 24, 2026 07:08
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Automated review conclusion: no blocking or correctness issues found. Approval opinion from this code review.
Critical checkpoint conclusions:
- Goal, scope, and tests: the PR cleanly moves
AdaptiveBlockSizePredictorfrom Storage to Core, updates all three production consumers and the focused unit test, removes only unused storage-specific declarations, and leaves prediction behavior unchanged. The Core and BE-test recursive CMake globs discover the new paths, and aggregate BE link sets include Core. - Concurrency and lifecycle: unchanged. Each FileScanner, FileScannerV2, or SegmentIterator owns one predictor through
unique_ptrand uses it on its existing sequential scan path. No new shared state, locks, atomics, static initialization, or release path is introduced. - Error handling, memory, and data correctness: no Status path, allocation policy, COW/nullable handling, transaction, persistence, visibility, or data-write behavior changes. The predictor remains a fixed-size per-instance helper.
- Compatibility and parallel paths: legacy file scan, V2 file scan, and segment scan include the new header; repository and exact-head searches found no stale old path or remaining
ColumnMetadatause. The const-qualified declaration and definition match every caller. No wire, storage-format, FE/BE protocol, configuration, or rolling-upgrade contract changes. - Performance and observability: runtime arithmetic and call frequency are unchanged; the patch removes one dead local vector and adds no logging, metrics, or hot-path work.
- Test/result review: the predictor behavior suite is preserved under
be/test/core/blockand remains discoverable by the existing GTest filter. Changed expectations and includes are type/style cleanup only.
User focus: no additional focus was supplied.
Validation scope: static review only because this review runner prohibits builds and test execution. The author reports 26 focused unit tests passed; I did not rerun them. At the pre-submission check, the live macOS BE UT job was still in progress, while formatting and the other completed visible checks were green.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 23542 ms |
Contributor
TPC-DS: Total hot run time: 121126 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
ClickBench: Total hot run time: 18.98 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#62835 Problem Summary: AdaptiveBlockSizePredictor is shared by segment scans and external file scanners, but its declaration and implementation were owned by storage/segment. Move the predictor and its focused unit test to core/block, update all consumers, and remove unused segment-only metadata declarations so the core header no longer depends on storage. ### Release note None ### Check List (For Author) - Test: - Unit Test: ASAN focused harness compiled the unchanged production and test sources directly (25 tests passed) - Manual test: targeted ASAN_UT compilation of the predictor, test, and all three consumers; clang-format, check-format, changed-line clang-tidy, and diff check - Standard `run-be-ut.sh` did not complete because this worktree's third-party installation is missing the unrelated `liblance_c.a` - Behavior changed: No - Does this need documentation: No
mrhhsg
force-pushed
the
refactor/move-adaptive-block-size-predictor
branch
from
July 28, 2026 09:32
cada6e6 to
62f455b
Compare
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review conclusion: no blocking or correctness issues found. Approval opinion from this code review.
Critical checkpoint conclusions:
- Goal and test proof: the PR moves
AdaptiveBlockSizePredictorand its focused 25-test suite fromstorage/segmenttocore/block, updates all three production consumers, removes only unused segment-specific declarations, and preserves the prediction body. Recursive CMake source/test discovery covers both new paths. - Scope: the patch is small and focused. The non-path edits are the valid
constqualification, removal of an unusedColumnMetadatadeclaration/local, explicit test includes, and type/style cleanup. - Concurrency: unchanged. Each
FileScanner,FileScannerV2, orSegmentIteratorowns one predictor and uses it on the existing sequential scan path; no shared state, lock, atomic, or thread-entry behavior changes. - Lifecycle: unchanged. Ownership remains a per-consumer
unique_ptr; no static/global initialization, release ordering, or cleanup path is introduced. - Configuration and compatibility: no configuration, wire protocol, storage format, serialization, class layout, installed/public ABI, or rolling-upgrade boundary changes. The const-qualified declaration/definition match every rebuilt in-tree caller.
- Parallel and conditional paths: segment scan, legacy file scan, and V2 file scan all use the new header; current/base reference closure found no stale old path or remaining
ColumnMetadatause. No runtime condition changes. - Build/link behavior: Core and BE-test recursive
CONFIGURE_DEPENDSglobs discover the moved files. Linux groups the internal archives, while Appleldcontinually searches static archives, so the Core-before-Exec/Storage order is valid on macOS. The liveCOMPILEcheck passed at the reviewed head. - Test coverage and results: all 25 focused predictor cases remain in the discovered test target, and their behavioral assertions are unchanged. The author reports a focused ASAN pass and targeted compilation of all three consumers; this review runner did not execute builds or tests. At submission, BE UT and macOS BE UT were still pending.
- Error handling, memory, data correctness, persistence, and writes: no status path, allocation/accounting policy, COW/nullable handling, transaction, visibility, persistence, or data-write behavior changes.
- Observability and performance: runtime arithmetic, call frequency, counters, and logging are unchanged; the patch removes one dead local vector and adds no hot-path work.
- Other concerns: no additional issue was found after the normal full-review and relocation-risk convergence scans.
User focus: no additional focus was supplied; the whole PR was reviewed.
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29345 ms |
Contributor
TPC-DS: Total hot run time: 176536 ms |
Contributor
ClickBench: Total hot run time: 25.04 s |
Gabriel39
approved these changes
Jul 31, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
gavinchou
approved these changes
Jul 31, 2026
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
4 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: #62835
Problem Summary:
AdaptiveBlockSizePredictoris a block-level helper shared by segment scans and external file scanners. Keeping it understorage/segmentforces external scan code to depend on the storage layer even though the predictor only usesBlockstatistics and EWMA calculations.This PR moves the predictor implementation and its focused unit test to
core/block, updates all consumers to use the new include path, and removes the unused segment-specificColumnMetadatadeclaration. It also makespredict_next_rows()const because it does not modify predictor state. The prediction behavior is unchanged.The branch has been rebuilt on the current
master, preserving the later external-scan rollback from #65998 rather than restoring the oldupdate(rows, bytes)path.Release note
None
Check List (For Author)
Test
AdaptiveBlockSizePredictorTest.*(25 tests passed)./run-be-ut.sh --run --filter='AdaptiveBlockSizePredictorTest.*' -j 48runner did not complete because this worktree's third-party installation is missing the unrelatedliblance_c.a; CI will provide the full monolithic BE validationSegmentIterator,FileScanner, andFileScannerV2)./build-support/clang-format.sh./build-support/check-format.shNOLINTENDinbe/src/core/types.hgit diff --check origin/master...HEADBehavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)