[fix](fe) Fix duplicate join nodes in PushDownAggThroughJoinOnPkFk tree reconstruction#65172
Open
starocean999 wants to merge 2 commits into
Open
[fix](fe) Fix duplicate join nodes in PushDownAggThroughJoinOnPkFk tree reconstruction#65172starocean999 wants to merge 2 commits into
starocean999 wants to merge 2 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Automated review completed. I found one issue in the new negative test: it does not isolate the intended no-pushdown scenario and it lacks a negative assertion against the pushed-down shape, so the regression can miss the root FK path.
Critical checkpoint conclusions:
- Goal/test proof: the rule change is focused on tree-join reconstruction, but the new negative test does not fully prove the intended behavior.
- Scope: the production change is narrow and localized to
InnerJoinCluster.constructPlan; no unrelated refactor was identified. - Concurrency/lifecycle/config/persistence: not applicable for this FE rewrite/test-only change.
- Parallel paths: reviewed the optimizer rule registration, FK elimination ordering, pairwise join collection, and aggregate pushdown paths; no additional substantiated optimizer correctness issue was found.
- Tests: static review only. FE tests were not run because
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare missing in this runner. - User focus: no additional user-provided focus was supplied.
Subagent conclusions: optimizer-rewrite found no candidates. tests-session-config proposed TSC-1; I accepted it as M-1 with narrowed rationale and dismissed the unsupported old-memo-alternative part. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the same final ledger/comment set.
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed PR #65172 statically. I found one blocking issue in the tree-join aggregate pushdown coverage/fix: the rule can still abort on an earlier ineligible PK/FK edge before trying a later valid edge.
Critical checkpoint conclusions:
- Goal/test: the PR targets duplicate join tree reconstruction, but the new positive tree-join test misses the early-abort path covered by the inline comment.
- Scope: changes are narrowly scoped to the Nereids rewrite and its unit test.
- Concurrency/lifecycle/config/persistence: not involved; no new config, persistence, or lifecycle surface.
- Parallel paths: multiple PK/FK edges in the same join cluster remain order-dependent because
newAgg == nullaborts the whole rule. - Testing: existing prior thread on the negative assertion is not duplicated; current head changed that SQL and added
nonMatch. A new regression is still needed for MAIN-1. - Validation:
git diff --check FETCH_HEAD..HEADpassed. FE build/tests were not run becausethirdparty/installed/bin/protocis missing, andfe/AGENTS.mdsays to stop before FE builds in that state.
Subagent conclusions: optimizer-rewrite proposed OPT-1, accepted as MAIN-1 and submitted inline. tests-session-config reported no candidates. Convergence round 1 ended with both live subagents replying NO_NEW_VALUABLE_FINDINGS for the unchanged final comment set.
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Automated review summary for current head 463c3c02c92670a46fce5d4bf63b09762393c916.
I reviewed the two changed files and the related Nereids rewrite path. I do not see a new substantiated blocking issue to raise on the current patch. The two existing inline review threads are not repeated here because the current head addresses them: newAgg == null now skips only the current PK/FK candidate, and the tree-join negative test removes the alternate PK/FK path and adds a negative pushed-down-shape assertion.
Critical checkpoint conclusions:
- Goal/test: the patch targets tree reconstruction in
PushDownAggThroughJoinOnPkFkplus the order-dependent abort when an earlier PK/FK edge cannot rewrite. The production change and added unit tests cover those scenarios at the plan-shape level. - Scope: the change is limited to the Nereids rule and its FE test.
- Concurrency/lifecycle/config/persistence/data writes/FE-BE protocol compatibility: not applicable; this is a logical optimizer rewrite and test-only schema setup.
- Parallel paths: both rule entry shapes remain covered (
Aggregate(Join)andAggregate(Project(Join))), and the multi-join PK/FK candidates are now skipped per candidate instead of aborting the whole rule. - Test coverage: new FE unit coverage was added for tree-join pushdown, disconnected non-primary reconstruction, and skipping an ineligible edge before succeeding on a later edge. I did not run FE tests because this checkout lacks
.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protoc. - Style/basic checks:
git diff --checkon the changed files passed. - User focus: no additional user-provided review focus was supplied.
Subagent conclusions: optimizer-rewrite and tests-session-config found no new valuable non-duplicate candidates. In convergence round 1, both returned NO_NEW_VALUABLE_FINDINGS for the same ledger and empty proposed inline comment set.
Contributor
TPC-H: Total hot run time: 29905 ms |
Contributor
TPC-DS: Total hot run time: 174444 ms |
Contributor
ClickBench: Total hot run time: 25.35 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
starocean999
requested review from
924060929,
englefly and
morrySnow
as code owners
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR #65172 at head b2a20e97f76ca03b29c0bd742f02db612678f225.
I reviewed the two changed files and the surrounding Nereids rewrite/test helpers:
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownAggThroughJoinOnPkFk.javafe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/PushDownAggThroughJoinOnPkFkTest.java
No new inline issue remains after duplicate suppression. The existing review threads already covered the earlier order-dependent newAgg == null abort, unstable HashMap iteration, invalid tree-join regression SQL, and childLeaves[1] == sharedLeaf side-selection concern; current head contains corresponding fixes.
Critical checkpoint conclusions:
- Goal and proof: The PR targets duplicate/incorrect tree reconstruction in
PushDownAggThroughJoinOnPkFkand the order-dependent abort while scanning flattened PK/FK edges. The implementation now continues past ineligible edges, uses deterministicLinkedHashMapjoin iteration, records child-leaf order, and reconstructs connected non-primary leaves without duplicating the accumulated subtree. Added FE unit cases cover the tree-join pushdown and skip-ineligible-edge paths by static inspection. - Scope: The change is limited to the Nereids rewrite rule and its focused FE unit tests.
- Concurrency/lifecycle/config/persistence/data writes: No relevant concurrency, lifecycle, persisted metadata, FE-BE protocol, or data-write path is introduced.
- Parallel paths: Both aggregate-over-join and aggregate-over-all-slot-project-over-join enter the same
pushAggpath, so the production fix applies to both rule wrappers. - Special conditions: Existing inner-join, non-mark-join, empty-other-conjunct gates are preserved.
InnerJoinCluster.isValid()still rejects joins whose conditions span more than two flattened leaves, and the final rewritten root checks that child outputs cover join input slots. - Tests: The added tests use the existing FE unit-test style and disable
PRUNE_EMPTY_PARTITION,ELIMINATE_JOIN_BY_FKconsistently withSessionVariableparsing. I did not run FE tests or checkstyle in this runner because.worktree_initialized,thirdparty/installed, andthirdparty/installed/bin/protocare absent, and the local checkout lacks the base commit, so validation was static plus GitHub PR metadata. - Observability/performance: No additional observability is needed for this planner rewrite. The added reconstruction work is bounded by the flattened join cluster size and uses existing
BitSet/plan traversal patterns. - Other review points: I did not find a distinct correctness, compatibility, or test issue worth a new inline comment.
Subagent conclusions:
optimizer-rewrite: no new candidate; checkedpushAgg,eliminatePrimaryOutput,collectContiguousInnerJoins,constructJoinWithPrimary, andconstructPlan.tests-session-config: no new candidate; checked FE unit-test setup, PK/FK constraints, disabled-rule propagation, assertion shape, and basic style hazards.- Final convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same current ledger/comment set after the last main ledger update.
User focus: .code-review.Axovz2/review_focus.txt had no additional user-provided review focus.
yx-keith
previously approved these changes
Jul 7, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29780 ms |
Contributor
TPC-DS: Total hot run time: 182264 ms |
Contributor
ClickBench: Total hot run time: 25.14 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
…ee reconstruction fix comment fix comment fix comment
github-actions
Bot
removed
the
approved
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at Jul 10th, 2026 6:44 AM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 29559 ms |
Contributor
TPC-DS: Total hot run time: 181235 ms |
Contributor
ClickBench: Total hot run time: 24.91 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR #65172.
I reviewed the two changed Nereids files and the surrounding PushDownAggThroughJoinOnPkFk rewrite path. I did not find a new, distinct inline issue to add beyond the existing review threads.
Critical checkpoint conclusions:
- Goal and tests: The patch addresses the duplicate join-tree reconstruction path and the abort-on-first-ineligible-PK/FK-edge path. The current code skips ineligible aggregate rewrites with
continue, uses deterministic join-edge iteration, and adds focused FE tests for the tree-join cases. - Scope and focus: The change is limited to one optimizer rewrite and its FE unit test. No configuration, persistence, transaction, FE-BE protocol, lifecycle, or concurrency path is introduced.
- Parallel paths: I checked the direct aggregate-over-join and aggregate-over-slot-project join patterns, plus flattened multi-join reconstruction. I did not find another changed path requiring an inline comment.
- Existing threads: The earlier abort-on-first-edge and weak tree-test-shape concerns are already covered by existing inline threads. The current patch appears to address those scenarios, so I did not duplicate them.
- Tests and CI: Local FE test execution was not attempted because this checkout lacks
thirdparty/installed/bin/protoc. GitHub status for the current head shows FE UT and CheckStyle success; legacy commit status also shows External Regression failed and P0/NonConcurrent pending, which I could not attribute to the changed files from static review alone. - Observability and performance: No new logging/metrics need was introduced, and I did not find a substantiated hot-path regression in the changed code.
- User focus: No additional user-provided review focus was supplied.
Subagent conclusions:
optimizer-rewrite: no new valuable findings; suspicious optimizer/tree-reconstruction points were already covered by existing threads or dismissed with code evidence.tests-session-config: no new valuable findings after checking tests, session-rule setup, and CI/style signals.- Final convergence: round 2 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same ledger and no-inline comment set after the CI caveat update.
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
5 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: #36035
Problem Summary:
This PR fixes two bugs in the PushDownAggThroughJoinOnPkFk rule that handles
pushing aggregates through joins using PK-FK constraints on multi-table joins.
Bug 1: Duplicate join nodes in tree reconstruction
When
constructPlanrebuilt a subtree from flattened inner joins, the mergepath did
currentPlan.withChildren(currentPlan, entryJoin), which insertedcurrentPlanas a child of itself and also keptentryJoinwith its originaltwo children. This produced duplicate join nodes and duplicate leaf references
in the resulting plan tree. The fix computes the new leaf set (
newBits = entryBitset - currentBitset) and only attaches the genuinely new child, usingentryJoin.withChildren(newChild, currentPlan)instead.Bug 2: First ineligible PK-FK edge aborts the entire rule
pushAggiterates all flattened PK-FK edges, but wheneliminatePrimaryOutputreturned null for an earlier edge, the code did
return null— aborting thewhole rule instead of continuing to try later edges. Since
HashMap<BitSet>iteration order on Java 17 visits the lower
{t_primary, t_foreign1}edgebefore the root
{t_other_primary, t_primary}edge, a query aggregating aprimary-side column (e.g.,
SUM(t_primary.pk_id)) would fail on the firstedge and never reach the valid root edge. Fixed by changing
return nulltocontinue.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)