[fix](fe) Preserve operative slots when deep copying logical relations#63315
Open
foxtail463 wants to merge 1 commit into
Open
[fix](fe) Preserve operative slots when deep copying logical relations#63315foxtail463 wants to merge 1 commit into
foxtail463 wants to merge 1 commit into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
|
/review |
englefly
approved these changes
May 18, 2026
Contributor
|
/review |
Contributor
|
run buildall |
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. |
![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: no blocking issues found in this PR.
Critical checkpoint conclusions:
- Goal and proof: The change fixes deep-copy remapping for operative slots whose index is outside the operative-slot list prefix by searching the full relation output. The added unit test covers a non-first operative slot and would fail on the old bound.
- Scope: The modification is minimal and focused on the incorrect loop bound plus a narrow regression test.
- Concurrency and lifecycle: This copier path does not introduce shared mutable state, locks, threads, or non-trivial lifecycle/static-initialization concerns.
- Configuration and compatibility: No new config, persistence, protocol, storage format, or rolling-upgrade compatibility concern.
- Parallel paths: The helper is used by
LogicalCatalogRelationcopy path, so the fix covers catalog scan subclasses that rely on relation output-to-output positional remapping. - Conditional checks: No new special conditional logic; existing missing-slot warning behavior is unchanged.
- Test coverage: A focused FE unit test was added for the bug scenario. I attempted to run
./run-fe-ut.sh --run org.apache.doris.nereids.trees.copier.LogicalPlanDeepCopierTest, but the runner lacksthirdparty/installed/bin/protoc, so generated-code setup failed before executing tests. - Observability, transactions, data writes, FE/BE variable passing, and performance: Not applicable to this small copier/test change; no additional issue found.
User focus points: No additional user-provided review focus was present.
Contributor
There was a problem hiding this comment.
No blocking issues found.
Critical checkpoint conclusions:
- Goal/test: The change fixes preservation of operative slots whose output position is beyond the number of operative slots, and the new unit test covers the reported non-first-slot case.
- Scope: The code change is minimal and focused on the incorrect search bound in LogicalPlanDeepCopier.
- Concurrency/lifecycle/config/compatibility: Not applicable; this is local plan-copy logic with no new shared state, lifecycle management, config, persistence, or wire/storage format change.
- Parallel paths: The change applies to LogicalCatalogRelation copier handling, which is the affected path for catalog relations with operative slots.
- Error handling: Existing warning path for unmatched operative slots remains unchanged.
- Test coverage: Added FE unit coverage for the regression scenario. No additional focus points were provided, and no extra issue was found there.
- Performance: The loop now scans the relation output instead of the operative-slot subset; this is necessary for correctness and remains small/local to plan copying.
- Data correctness: Preserving operative slots avoids losing required materialization information after deep copy.
Review context: Existing inline review threads were empty, so no duplicate issues were present.
Contributor
TPC-H: Total hot run time: 31344 ms |
Contributor
TPC-DS: Total hot run time: 168820 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
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.
problem summary:
When deep copying a
LogicalCatalogRelation,LogicalPlanDeepCopier#updateOperativeSlotsneeds to remap the original relation's operative slots to the corresponding slots in the copied relation.Before this change, the search range was incorrectly limited by
oldOperativeSlots.size()instead of the relation output size. If the operative slot was not located in the firstoldOperativeSlots.size()output positions, the copier could not find the matching slot and the copied relation would lose that operative slot.For example, when a relation has multiple output slots but only one operative slot at index 1, the old logic only checked index 0 and failed to preserve the operative slot.