[refactor](be) Remove redundant remaining conjunct roots#63525
Merged
gavinchou merged 1 commit intoMay 25, 2026
Merged
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: SegmentIterator kept a separate remaining_conjunct_roots list in addition to common_expr_ctxs_push_down. Both represented the same residual common expression state, so index pruning paths had to keep them synchronized and could drift when an expression was removed after index evaluation. This change removes the duplicated remaining_conjunct_roots plumbing and uses common_expr_ctxs_push_down as the single source of residual common expression state for condition cache decisions, lazy materialization column extraction, and expression execution checks.
### Release note
None
### Check List (For Author)
- Test: Build
- ./build.sh --be
- build-support/clang-format.sh
- git diff --check
- Behavior changed: No
- Does this need documentation: No
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.
Review result: no blocking findings identified. The refactor consistently removes the duplicated remaining_conjunct_roots plumbing and uses common_expr_ctxs_push_down as the single residual common-expression state across scan params, tablet/rowset reader context, SegmentIterator condition-cache gating, index-pruned expression removal, lazy-materialization column extraction, and expression execution checks.
Critical checkpoint conclusions:
- Goal/test: The code accomplishes the stated refactor goal; no leftover references to the removed fields were found. The PR reports BE build/format/diff-check, but I did not rerun a full build in this review.
- Scope: The change is small and focused on one data path and removes duplicated state rather than adding new behavior.
- Concurrency/lifecycle: No new threads, shared mutable cross-thread state, locks, or special lifecycle/static initialization concerns are introduced.
- Configuration/compatibility/protocol: No new config, persisted format, wire protocol, or rolling-upgrade compatibility impact found.
- Parallel paths: The scan -> tablet reader -> rowset reader -> segment iterator forwarding path is updated consistently for the removed field.
- Conditional logic: The remaining conditional checks now use common_expr_ctxs_push_down directly; this matches the post-index residual-expression state used for execution.
- Test coverage/results: No test files were added or changed in the actual PR diff. Given this is a narrow refactor, build coverage is plausible, but an execution/regression case around common expr pushdown with inverted/ANN index pruning would further reduce risk.
- Observability: No new user-visible path or operational state requiring additional logging/metrics was introduced.
- Transactions/persistence/data writes: Not applicable; this is read-path scan filtering state only.
- FE/BE variables: No FE-BE thrift or transmitted variable changes found.
- Performance: The change removes duplicate vector state and synchronization work; no obvious new hot-path overhead was found.
User focus points: none were provided.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31259 ms |
Contributor
TPC-DS: Total hot run time: 170104 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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. |
yiguolei
approved these changes
May 25, 2026
Contributor
|
/review |
Contributor
There was a problem hiding this comment.
Review result: no blocking issues found. The change keeps common_expr_ctxs_push_down as the single residual common-expression state and removes duplicated remaining_conjunct_roots plumbing without leaving source references behind.
Critical checkpoint conclusions:
- Goal/test: The goal is a focused BE refactor to eliminate duplicated residual common-expression state. The code accomplishes this by passing only
common_expr_ctxs_push_downthrough scanner/tablet/rowset/segment readers. PR reports./build.sh --be,build-support/clang-format.sh, andgit diff --check; no new functional regression test is added, which is acceptable for this narrow internal refactor but leaves runtime path coverage dependent on existing tests. - Scope/focus: The modification is small and focused on the redundant state.
- Concurrency/lifecycle: No new threads, shared mutable state, locks, or non-obvious static/global lifecycle changes were introduced. Segment-local expression context cloning remains per existing flow.
- Config/compatibility: No new config, protocol, storage format, or compatibility-impacting changes.
- Parallel paths: The FE-to-BE scan reader plumbing path was updated consistently across
OlapScanner,TabletReader,RowsetReaderContext,StorageReadOptions, andSegmentIterator. - Conditional logic: Condition cache, inverted-index/ANN expression erasure, lazy materialization column extraction, and residual expression execution now all use the same remaining
common_expr_ctxs_push_downlist. This appears consistent with the intended source of truth. - Testing/result files: No test result files are modified. Reported build/format checks are appropriate for the refactor; I did not run additional tests in this review environment.
- Observability: No new observability is needed for this internal state cleanup. Existing VLOG/profile/stat counters remain unchanged.
- Transaction/persistence/data writes: Not involved.
- FE-BE variables: No new transmitted variables are added; one internal read-option field is removed through all affected call sites.
- Performance: Removing the duplicate vector and synchronization work is neutral to positive; no obvious hot-path regression found.
User focus: no additional user-provided review focus was present.
gavinchou
approved these changes
May 25, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
May 25, 2026
Problem Summary: SegmentIterator kept a separate `remaining_conjunct_roots` list in addition to `common_expr_ctxs_push_down`. Both represented the same residual common expression state, so index pruning paths had to keep them synchronized and could drift when an expression was removed after index evaluation. This change removes the duplicated `remaining_conjunct_roots` plumbing and uses `common_expr_ctxs_push_down` as the single source of residual common expression state for condition cache decisions, lazy materialization column extraction, and expression execution checks.
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
5 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: SegmentIterator kept a separate
remaining_conjunct_rootslist in addition tocommon_expr_ctxs_push_down. Both represented the same residual common expression state, so index pruning paths had to keep them synchronized and could drift when an expression was removed after index evaluation. This change removes the duplicatedremaining_conjunct_rootsplumbing and usescommon_expr_ctxs_push_downas the single source of residual common expression state for condition cache decisions, lazy materialization column extraction, and expression execution checks.Release note
None
Check List (For Author)
./build.sh --bebuild-support/clang-format.shgit diff --check