[refactor](be) Remove redundant remaining conjunct roots#63525
Draft
mrhhsg wants to merge 1 commit into
Draft
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
|
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 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