[fix](lazy topn) V2: Fix slot-not-found after PullUpProjectExprUnderTopN with chained expressions #64486
Open
englefly wants to merge 2 commits into
Open
[fix](lazy topn) V2: Fix slot-not-found after PullUpProjectExprUnderTopN with chained expressions #64486englefly wants to merge 2 commits into
englefly wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29320 ms |
Contributor
TPC-DS: Total hot run time: 169457 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
englefly
requested review from
924060929,
morrySnow and
starocean999
as code owners
Contributor
TPC-H: Total hot run time: 29047 ms |
Contributor
TPC-DS: Total hot run time: 169006 ms |
Contributor
Author
|
run cloud_p0 |
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.
I found one blocking correctness issue in the new bottom-up rewrite. The rule can now move aliases that canPullUp explicitly rejects when those aliases depend on a lower alias that was pulled up.
Critical checkpoint conclusions:
- Goal/test: The PR aims to fix slot-not-found/chained-expression behavior for
PullUpProjectExprUnderTopN; the added unit tests cover several nested TopN and rename cases, but miss the chained non-movable/volatile case below. - Scope: The change is focused to the Nereids rewrite and its unit test, but the replacement-map broadening changes semantic eligibility beyond the stated fix.
- Concurrency/lifecycle/config/compatibility/persistence: Not applicable; this is a logical optimizer rewrite with no new config, storage, protocol, or transaction persistence changes.
- Parallel paths: The affected path is the custom TopN expression pull-up rule used before lazy materialization; the lower lazy-materialization post processor still relies on this rule preserving expression movability constraints.
- Tests: I attempted
./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.PullUpProjectExprUnderTopNTest, but it failed before test execution becausethirdparty/installed/bin/protocis missing in this runner. - Observability/performance: No additional observability needed; performance intent is reasonable once the semantic guard is restored.
User focus: No additional user-provided review focus was present.
| @@ -237,37 +170,27 @@ public Plan visitLogicalTopN(LogicalTopN topN, CollectorContext context) { | |||
| * along the path from the TopN to the current node. An expression whose output ExprId | |||
| * is in this set cannot be pulled up past the operators that reference it. | |||
| */ | |||
Contributor
There was a problem hiding this comment.
This records every alias in the replacement map, but later simplifyProject removes any alias whose inputs became unavailable and addUpperProject reconstructs it above the TopN. That bypasses canPullUp for aliases that were intentionally rejected. For example, with TopN -> Project(z = assert_true(x > 0)) -> Project(x = a + 1) -> Scan, x is pullable, so the lower project is simplified and x disappears from the upper project's child. Because z was also placed in pullUpExprReplaceMap, the upper project is removed and z = assert_true(a + 1 > 0) is restored above the TopN, even though AssertTrue is a NoneMovableFunction and canPullUp would have returned false for z. The same pattern applies to volatile/Score/L2Distance aliases. This can suppress errors or change semantics for rows discarded by TopN, so the replacement map needs to distinguish aliases that are safe to synthesize above TopN from aliases that must remain below it.
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
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
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: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)