[fix](fe) Prune empty and cascading CTE plans#62828
Open
englefly wants to merge 4 commits into
Open
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
|
OpenCode automated review failed and did not complete. Error: Review step was failure (possibly timeout or cancelled) Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
|
/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 2 blocking issues.
Critical checkpoint conclusions:
- Goal of this PR: partially accomplished. The change does address empty/cascading CTE pruning in the optimizer path, and the new regression suite covers two representative plan shapes, but one fixpoint bug in the new normalization loop means cascading cleanup can still stop too early.
- Scope/minimality: mostly focused, but the
SessionVariableedit also removes a legacy variable alias unrelated to the optimizer fix. - Concurrency: no concurrency or locking concerns are introduced in these touched FE optimizer/session-variable paths.
- Lifecycle/static-init: no special lifecycle or static initialization issues are involved here.
- Configuration items: no new config was added, but an existing session variable declaration was changed in a user-visible way.
- Compatibility: not preserved. Removing
cbo_cte_inline_modebreaks existingSET/SHOW VARIABLESclients that still use the old alias. - Parallel code paths: the new optimizer-side normalization path is distinct from the main rewrite path, and its fixpoint detection currently relies on shallow
equals()semantics that do not reflect subtree changes. - Special conditions/checks: the new
currentPlan.equals(normalizedPlan)stop condition is not strong enough for Nereids plans. - Test coverage: improved for empty/cascade pruning, but still missing coverage for the shallow-equality fixpoint case and for preserving the legacy session-variable alias.
- Test result updates: one new regression test file was added; I did not run the test suite locally in this runner.
- Observability: no additional observability appears necessary for this change.
- Transaction/persistence/data-write/FE-BE variable passing: not applicable for this PR.
- Performance: no blocking performance issue identified beyond the correctness problems above.
- Other issues: none beyond the two findings below.
User focus: /tmp/review_focus.txt contains no additional focus items, so there were no extra focus-specific findings to report.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
I did not add duplicate inline comments because the remaining blocking issue is already covered by the existing review thread on SessionVariable.java: removing the cbo_cte_inline_mode alias is a user-visible compatibility break and should be restored or intentionally documented with a migration plan. The earlier Plan.equals() fixpoint issue appears addressed by the explicit changed flag in the current head.
Critical checkpoint conclusions:
- Goal/test: The PR targets CTE pruning and cascading inline normalization, with regression coverage added for empty and cascading CTE cases.
- Scope: The optimizer changes are focused, but the alias removal is unrelated to the optimizer fix and should not be included.
- Concurrency/lifecycle: No new concurrent mutable state, locks, or special lifecycle hazards identified.
- Configuration/compatibility: Compatibility issue remains for the removed session-variable alias.
- Parallel code paths: The new normalization is applied to the base rewrite plan and CBO inline alternatives; no distinct missing path found.
- Testing: Regression coverage exists for the main CTE pruning behavior; no additional user focus points were provided.
- Observability/persistence/data writes: Not applicable for this optimizer-only change.
- Performance: No blocking performance issue found in the changed code; the normalization loop is bounded by CTE removal/inlining progress.
Focus response: review_focus.txt had no additional focus points.
Contributor
Author
|
run buildall |
1 similar comment
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29234 ms |
Contributor
TPC-H: Total hot run time: 29546 ms |
Contributor
TPC-DS: Total hot run time: 170284 ms |
Contributor
TPC-DS: Total hot run time: 170993 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29539 ms |
Contributor
TPC-DS: Total hot run time: 171356 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29596 ms |
Contributor
TPC-DS: Total hot run time: 170170 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Issue Number: None
Related PR: None
Problem Summary: Fix optimizer-side CTE pruning so empty-relation producers, zero-consumer anchors, and cascading inline opportunities are normalized to a fixpoint before memoization, and add regression coverage for empty and cascading CTE elimination.
None
- Test: No need to test (user requested to skip compile and test)
- Behavior changed: Yes (empty and cascade-pruned CTEs are normalized before memoization)
- Does this need documentation: No
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
[fix](fe) Fix cascading CTE normalize fixpoint
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: CTE normalization used Plan.equals() as the fixpoint check. Some logical plan nodes do not compare children in equals(), so CTE rewrites under a kept parent node could be missed and cascading inline/prune normalization could stop too early.
Fix cascading CTE pruning/inline normalization when child CTE changes occur under logical nodes whose equals() does not compare children.
- Test: Regression test / FE checkstyle
- ./run-regression-test.sh --run -d query_p0/cte -s test_cbo_cte_inline_prune
- cd fe && mvn checkstyle:check -pl fe-core
- Behavior changed: Yes. CTE normalization now uses explicit rewrite state instead of shallow Plan.equals() for fixpoint detection.
- Does this need documentation: No
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
[fix](regression) Update repeat CTE shape expectation
Issue Number: close #xxx
Related PR: #xxx
Problem Summary: CTE normalization now inlines the single-consumer CTE in the repeat output slot shape case, so the expected shape output needed to be updated while query results stay unchanged.
None
- Test: Regression test
- ./run-regression-test.sh --run -d query_p0/repeat -s test_repeat_output_slot
- Behavior changed: No. Test expectation only.
- Does this need documentation: No
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
fix-shape
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29448 ms |
Contributor
TPC-DS: Total hot run time: 170881 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29687 ms |
Contributor
TPC-DS: Total hot run time: 170622 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29297 ms |
Contributor
TPC-DS: Total hot run time: 168555 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29355 ms |
Contributor
TPC-DS: Total hot run time: 171694 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run cloud_p0 |
Contributor
Author
|
/review |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
There was a problem hiding this comment.
I found one additional issue beyond the existing review threads. The existing alias-removal compatibility concern in SessionVariable also still appears unresolved in the current diff, but I did not duplicate that inline comment.
Critical checkpoint conclusions:
- Goal/test: the PR targets CTE empty-prune and cascading inline normalization, and adds focused regression coverage, but the implementation leaves optimizer CTE bookkeeping stale after mutating the rewrite plan.
- Scope: the change is generally focused, but the new normalization hook needs to maintain existing
StatementContextinvariants. - Concurrency/lifecycle: no new concurrent execution path or special lifecycle issue found.
- Config/compatibility: no new config is added; the removed
cbo_cte_inline_modealias is an existing user-visible compatibility issue already raised in a prior thread. - Parallel paths: CTE consumer/producer bookkeeping paths used by costing and runtime filter generation are not updated after this new normalization path.
- Tests: regression tests cover plan-shape normalization, but they do not cover stale CTE consumer metadata effects on costing/runtime-filter decisions.
- Observability/transactions/persistence: not applicable.
- Performance: stale consumer counts can mis-cost remaining CTE producers/consumers and can suppress runtime-filter pushdown.
User focus: no additional user-provided review focus was supplied.
| public void execute() { | ||
| MoreFieldsThread.keepFunctionSignature(() -> { | ||
| Plan rewritePlan = cascadesContext.getRewritePlan(); | ||
| if (containsCte(rewritePlan)) { |
Contributor
There was a problem hiding this comment.
After this replaces the rewrite plan, the CTE bookkeeping in StatementContext still describes the pre-normalized tree populated by CTEInline/RewriteCteChildren. That metadata is used later by CostModel (getCteIdToConsumers() for the consumer count and getCteProducerByCteId() for producer shape) and by runtime-filter pushdown to decide whether all CTE consumers have filters. A concrete case is a CTE with three consumers where normalizeCtePlan() eliminates one empty branch but leaves the CTE materialized with two consumers: the memo is built from the two-consumer plan, while StatementContext.getCteIdToConsumers() still reports three, so the materialized CTE can be over-costed as the numConsumers >= 3 UNION ALL case and runtime-filter pushdown can be skipped because it waits for a non-existent third consumer. Please refresh/clear and rebuild the CTE producer/consumer context after normalization before toMemo() and CBO costing run.
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
3 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: #60601
Problem Summary: Fix optimizer-side CTE pruning so empty-relation producers, zero-consumer anchors, and cascading inline opportunities are normalized to a fixpoint before memoization, and add regression coverage for empty and cascading CTE elimination.
Release note
None
Check List (For Author)
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)