[opt](be) Cache segment row bitmap cardinality - #66742
Open
Mryange wants to merge 1 commit into
Open
Conversation
### What problem does this PR solve?
Issue Number: N/A
Problem Summary: SegmentIterator recalculates the row bitmap cardinality for every output batch. Large row bitmaps require traversing many Roaring containers for each calculation. Cache the cardinality after all lazy-init filters are applied and reuse it for column reservation and batch reads.
### Release note
None
### Check List (For Author)
- Test: No build or tests were run.
- clang-format 16 dry-run passed
- git diff --check passed
- Behavior changed: No. This only removes redundant cardinality calculations.
- Does this need documentation: No.
Mryange
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
August 13, 2026 12:48
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 opinion: approval; no blocking correctness issue found.
The change caches the final scan bitmap cardinality after all bitmap-producing filters and reuses that identical value for initial reservation and per-batch read sizing. The placement and one-shot iterator lifecycle preserve the prior semantics while avoiding repeated Roaring cardinality traversal.
Critical checkpoint conclusions:
- Goal and proof: the implementation accomplishes the stated performance goal at both repeated call sites. Existing scan behavior tests exercise adjacent LIMIT/reverse paths, but this PR adds no direct cache-equivalence or performance test.
- Scope and clarity: the change is small and focused to
SegmentIterator; the member name, initialization, and placement communicate the invariant clearly. - Concurrency: no new concurrent state is introduced. A
SegmentIteratoris scan-local and consumed through its existing one-shot iterator path; the cached member follows the same ownership as_row_bitmapand requires no new lock. - Lifecycle and static initialization: all bitmap mutation paths complete before cache publication. Failed lazy initialization retries rebuild the bitmap, cache, and range iterator because
_lazy_initedremains false; successful initialization is one-shot. No static/global initialization is involved. - Configuration: no configuration item is added or changed.
- Compatibility: no function symbol, persisted state, storage format, or FE-BE protocol changes are introduced, so rolling-upgrade compatibility is unaffected.
- Parallel and special paths: condition-cache pruning, key/column/inverted-index filtering, delete bitmaps, explicit row ranges, BM25 score filtering, ANN range/TOP-N, forward/reverse scans, adaptive batch sizing, pushed LIMIT, empty bitmaps, and EOF were traced. No post-publication bitmap mutation or stale-cache reuse was found.
- Tests and results: no test files or expected-result files are changed. Existing coverage includes relevant LIMIT and reverse-order semantics; no builds or tests were run in this review-only environment, as required by the review bundle.
- Observability: this internal reuse does not add a failure mode or distributed operation that needs new logging, metrics, or tracing.
- Transactions, persistence, and data writes: not applicable; the change only affects read-side batch sizing and does not alter visibility, delete-bitmap versions, atomicity, or crash recovery.
- Cross-process variables: no new value is transmitted between FE and BE.
- Performance and memory safety: the optimization removes repeated full cardinality calculations. The cached
uint64_tmatchesRoaring::cardinality(); the existingcast_set<uint32_t>boundary is unchanged, adaptive reads remain capped by_initial_block_row_max, and_block_rowidsremains sized to that ceiling. - Other issues: none substantiated after the full-scope and risk-focused review sweeps.
User focus: no additional focus was supplied; the entire two-file PR was reviewed.
Contributor
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 17277 ms |
Contributor
TPC-DS: Total hot run time: 81483 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.
SegmentIteratorrecalculates the row bitmap cardinality for every output batch. For large segments, this repeatedly traverses all Roaring bitmap containers and adds significant scan overhead. This change caches the cardinality after all bitmap filters are applied during lazy initialization and reuses it for column reservation and batch reads.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)