[fix](set operation) Use regular child output for set operation rules#64908
Open
morrySnow wants to merge 1 commit into
Open
[fix](set operation) Use regular child output for set operation rules#64908morrySnow wants to merge 1 commit into
morrySnow wants to merge 1 commit into
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#64900, apache#64906 Problem Summary: Some Nereids rewrite rules mapped set operation outputs to child outputs by list index. Set operations can carry regulator child outputs whose order and cardinality differ from the child plan output, so those rules could read past the child output or rewrite expressions with the wrong child slot. This change makes one-row union merge and distinct TopN/Limit pushdown build child mappings from regular child outputs instead. ### Release note Fix planner failures or incorrect set operation rewrites when regulated child outputs differ from child plan outputs. ### Check List (For Author) - Test: Regression test / Unit Test - ./run-fe-ut.sh --run org.apache.doris.nereids.rules.rewrite.SetOperationOutputMappingTest - cd fe && mvn checkstyle:check -pl fe-core - ./run-regression-test.sh --run -d nereids_rules_p0/push_down_top_n -s push_down_top_n_distinct_through_union - ./run-regression-test.sh --run -d nereids_rules_p0/push_down_limit_distinct -s push_down_limit_distinct_through_union - ./run-regression-test.sh --run -d nereids_rules_p0/merge_one_row_relation -s merge_one_row_relation_into_union - Behavior changed: Yes (fixes set operation rewrites to use regular child outputs) - Does this need documentation: No
morrySnow
requested review from
924060929,
englefly and
starocean999
as code owners
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review found one blocking issue: the new regression EXPLAIN cases use plain sql, so they execute but do not compare the shape plan golden output. Inline comments mark each instance.
Checkpoint conclusions:
- Goal/scope: The code change is focused on using
regularChildrenOutputsin set-operation rewrites, and the implementation path appears semantically consistent after tracingBindExpression.bindSetOperation,LogicalUnion, expression replacement, and physical union translation. - Tests: FE unit tests target the internal mapping, but the added regression coverage is incomplete because the new planner-session EXPLAIN statements are not
qt_checks and add no.outresults. - Concurrency/lifecycle/config/compatibility/persistence/storage/cloud: not involved by this FE optimizer/test-only PR.
- Parallel paths: one-row merge plus limit/topN distinct union paths were reviewed; no additional rewrite issue was found.
- Validation:
git diff --checkpassed. I did not run FE/regression tests locally becausethirdparty/installed/bin/protocis absent in this checkout.
Subagent conclusions:
- optimizer-rewrite: no optimizer semantic or mapping candidates; final convergence round returned
NO_NEW_VALUABLE_FINDINGS. - tests-session-config: TSC-1 accepted as the inline regression-test issue; final convergence round returned
NO_NEW_VALUABLE_FINDINGS.
User focus: no additional focus was provided.
| explain shape plan select * from ((select * from table2 t1 limit 5) union (select * from table2 t2 limit 5)) sub order by id limit 10; | ||
| """ | ||
|
|
||
| sql """ |
Contributor
There was a problem hiding this comment.
Please make this new EXPLAIN a named qt_... check and regenerate the corresponding .out block instead of running it with plain sql. sql only executes the EXPLAIN; it does not compare the shape plan, so this regression would still pass if the rewrite stopped pushing the intended shape as long as planning succeeds.
| ); | ||
| """ | ||
|
|
||
| sql """ |
Contributor
There was a problem hiding this comment.
Please make this EXPLAIN a named qt_... check and add the generated .out output instead of running it with plain sql. sql only executes the EXPLAIN; it does not compare the shape plan, so this regression would still pass if the rewrite stopped pushing the intended shape as long as planning succeeds.
| sql "SET enable_nereids_planner=true" | ||
| sql "SET enable_fallback_to_original_planner=false" | ||
|
|
||
| sql """ |
Contributor
There was a problem hiding this comment.
Please make this EXPLAIN a named qt_... check and add the generated .out output instead of running it with plain sql. sql only executes the EXPLAIN; it does not compare the shape plan, so this regression would still pass if the one-row merge stopped producing the intended shape as long as planning succeeds.
Contributor
TPC-H: Total hot run time: 29252 ms |
Contributor
TPC-DS: Total hot run time: 171538 ms |
Contributor
ClickBench: Total hot run time: 25.17 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
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?
Related PR: #24060, #27628
Problem Summary: Some Nereids rewrite rules mapped set operation outputs to child outputs by list index. Set operations can carry regulator child outputs whose order and cardinality differ from the child plan output, so those rules could read past the child output or rewrite expressions with the wrong child slot. This change makes one-row union merge and distinct TopN/Limit pushdown build child mappings from regular child outputs instead.
Release note
Fix planner failures or incorrect set operation rewrites when regulated child outputs differ from child plan outputs.
Check List (For Author)