[fix](fd)drop Function dependencies from join outer side - #65982
Merged
Conversation
englefly
requested review from
924060929,
morrySnow and
starocean999
as code owners
July 24, 2026 03:07
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
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.
Request changes: one P2 optimizer performance regression remains.
Critical checkpoint conclusions:
- Goal and correctness: the patch prevents unsafe direct functional dependencies from surviving a null-extended outer-join side, and no unsafe edge is reconstructed through unique, uniform, equality, transitive, or pruning paths. However, it also drops still-valid dependencies whose determinant contains an originally non-null child slot; see the inline finding.
- Scope and parallel paths: all 14 current join types, ASOF directionality, semi/anti/NAAJ and mark shapes were checked. The logical and physical hash-join switches agree; nested-loop recomputation blocks FD propagation. No additional path-specific issue was found.
- Tests: the changed FdTest assertions inspect only the logical plan and use NOT NULL determinants, so they codify the safe dependency loss rather than reproduce the nullable-determinant collision. The physical reset/recomputation path is not covered.
- Performance: the accepted issue prevents EliminateGroupByKey and EliminateOrderByKey from applying valid reductions. No other CPU, memory, or hot-path concern was found.
- Concurrency, lifecycle, configuration, compatibility, persistence/transactions, data writes, FE-BE propagation, and observability: not involved in this change.
- User focus: no additional focus was provided; the complete PR was reviewed.
Validation was static only. No build or test was run because the governing review prompt prohibits builds in this environment.
Contributor
TPC-H: Total hot run time: 23165 ms |
Contributor
TPC-DS: Total hot run time: 117085 ms |
Contributor
ClickBench: Total hot run time: 18.65 s |
morrySnow
previously approved these changes
Jul 27, 2026
Contributor
|
PR approved by at least one committer and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
github-actions
Bot
removed
the
approved
Contributor
There was a problem hiding this comment.
Request changes: one P1 correctness issue and two distinct P2 FD-preservation gaps remain. The existing thread also remains relevant; no duplicate inline was added for its composite-determinant and test-coverage points.
Critical checkpoint conclusions:
- Goal and correctness: the PR aims to retain exactly the functional dependencies that survive outer-join null extension.
MAIN-F1shows the implementation can retain an invalid historical-slot FD and letEliminateGroupByKeymerge distinct groups.MAIN-F2andMAIN-F3show it also drops safe transitive and null-rejected-key dependencies. - Scope and clarity: the five-file change is focused. Both logical and physical hash-join switches cover all 14 current join types with matching side propagation; no unrelated source change was found.
- Concurrency and lifecycle: no runtime concurrency, locks, shared mutable state, or static initialization are involved. The relevant optimizer-property lifecycle was traced through lazy logical traits and bottom-up physical recomputation; historical
Slotnullability in that lifecycle is the P1 issue. - Parallel and special paths: ordinary/right/full/ASOF outer joins, semi/anti/NAAJ, mark joins, implementation conversion, output pruning, and the FD-blocking nested-loop path were checked.
MAIN-F3is intentionally limited to a proven null-rejecting key on the non-preserved side of a one-sided join; full outer and null-safe/unproven predicates are excluded. - Tests: the positive logical assertions cover direct schema-non-null determinants. The new nullable-key negative assertions are vacuous because that scan never seeds the claimed FD, and no added test reaches physical recomputation; those points are already covered by the existing thread and were not duplicated inline.
- Configuration, compatibility, persistence, transactions, data writes, FE-BE propagation, error boundaries, and observability: not involved.
- Performance: graph copying remains linear, but
MAIN-F2andMAIN-F3prevent valid group/order-key reductions. - User focus: no additional review focus was provided; the whole PR was reviewed.
- Completion: three bounded rounds converged, with every final-round full and risk reviewer returning
NO_NEW_VALUABLE_FINDINGSagainst this exact comment set.
Validation was static only. No build or test was run because the governing review prompt prohibits builds in this environment.
Contributor
TPC-H: Total hot run time: 29678 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 177247 ms |
Contributor
ClickBench: Total hot run time: 24.96 s |
Contributor
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28424 ms |
Contributor
TPC-DS: Total hot run time: 166787 ms |
Contributor
ClickBench: Total hot run time: 23.81 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run external |
Contributor
FE Regression Coverage ReportIncrement line coverage |
feiniaofeiafei
approved these changes
Aug 7, 2026
starocean999
approved these changes
Aug 14, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Aug 14, 2026
### What problem does this PR solve?
Issue Number: N/A (no issue linked)
Related PR: N/A
Problem Summary:
Nereids derives functional dependencies (FDs) from each operator's
children via `DataTrait`, and rewrite rules such as
`EliminateGroupByKey`, `EliminateGroupByKeyByUniform`,
`EliminateOrderByKey` and `ConstantPropagation` consume these FDs to
drop functionally-determined grouping/ordering keys. If an FD is derived
incorrectly, those rules may produce wrong query results.
`LogicalJoin.computeFd()` and `PhysicalHashJoin.computeFd()` previously
propagated FDs from both join inputs, only excluding the semi/anti-join
side:
```java
if (!joinType.isLeftSemiOrAntiJoin()) {
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
}
if (!joinType.isRightSemiOrAntiJoin()) {
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
}
```
For outer joins the nullable side is null-extended: unmatched rows are
padded with NULLs, which invalidates FDs from that side. For example, in
`t1 LEFT OUTER JOIN t2`, if the right side has the FD `t2.a -> t2.b` and
`a` is nullable, a matched row with `a = NULL, b = 1` and an unmatched
row `(a = NULL, b = NULL)` together violate `a -> b` on the join output.
The old code still propagated such FDs from the nullable side for `LEFT
OUTER JOIN` (right side), `RIGHT OUTER JOIN` (left side) and `FULL OUTER
JOIN` (both sides), so a downstream rule could remove a group-by key
that is not actually functionally determined and change the query
result.
This PR fixes the FD derivation on join outputs:
1. `computeFd()` in `LogicalJoin` and `PhysicalHashJoin` is rewritten
with an explicit switch over join types:
- inner / cross joins: propagate FDs from both sides;
- semi / anti joins: propagate FDs only from the output side;
- outer joins: propagate FDs from the preserved side, and from the
nullable side only the FDs whose determinant is NOT NULL in the child —
matched rows then always carry a non-null determinant, so they cannot
collide with the `(NULL, NULL)` null-extension of unmatched rows;
- full outer join: keep only the NOT-NULL-determinant FDs from both
sides.
2. A new `DataTrait.Builder.addFuncDepsDGForOuterJoinNullableSide()` /
`FuncDepsDG.Builder.addDepsForOuterJoinNullableSide()` implements the
NOT-NULL-determinant filter.
3. The nullability check is performed against the *current* child output
rather than the slot stored in the FD graph: slots are keyed by ExprId
and may carry a stale `nullable` flag (e.g. after
`LogicalSubQueryAliasToLogicalProject` inlining), so a determinant that
became nullable in the immediate child is dropped.
Tests in `FdTest` are updated (FOJ/LOJ/ROJ no longer propagate
nullable-side FDs, while NOT-NULL-determinant FDs from the nullable side
are kept), and a new `testNestedOuterJoinNullableDeterminant` covers the
nested outer-join case where the determinant's stale non-nullable flag
must not leak through, verified on both the logical and the physical
join paths.
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
16 tasks
yiguolei
pushed a commit
that referenced
this pull request
Aug 15, 2026
englefly
added a commit
that referenced
this pull request
Aug 17, 2026
…rapping (#64849) ### What problem does this PR solve? When a group-by key is functionally dependent on another key (e.g. s_suppkey -> s_name via PK) but required in output, remove it from GROUP BY and wrap with ANY_VALUE(). Previously EliminateGroupByKey kept such keys in GROUP BY to preserve SQL semantics. Now they are replaced with ANY_VALUE wrappers in the output, allowing the group-by set to be minimized while keeping the column in SELECT. Public findCanBeRemovedExpressions() API preserved for backward compatibility. Internal logic split into FindResult with separate removeExpression and wrapWithAnyValue sets. Test: testEliminateByPkWithOutputNeeded verifies ANY_VALUE wrapping when SELECT contains an FD-redundant group-by key. Issue Number: close #xxx Related PR: #65982 #66801 #66803 上面 3 个 pr 是原有 master 的bug fix. pick 这个 pr 前, 确保上面 3 个 pr 已经 pick Problem Summary: ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [ ] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [ ] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [ ] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
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
6 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: N/A (no issue linked)
Related PR: N/A
Problem Summary:
Nereids derives functional dependencies (FDs) from each operator's children via
DataTrait, and rewrite rules such asEliminateGroupByKey,EliminateGroupByKeyByUniform,EliminateOrderByKeyandConstantPropagationconsume these FDs to drop functionally-determined grouping/ordering keys. If an FD is derived incorrectly, those rules may produce wrong query results.LogicalJoin.computeFd()andPhysicalHashJoin.computeFd()previously propagated FDs from both join inputs, only excluding the semi/anti-join side:For outer joins the nullable side is null-extended: unmatched rows are padded with NULLs, which invalidates FDs from that side. For example, in
t1 LEFT OUTER JOIN t2, if the right side has the FDt2.a -> t2.bandais nullable, a matched row witha = NULL, b = 1and an unmatched row(a = NULL, b = NULL)together violatea -> bon the join output. The old code still propagated such FDs from the nullable side forLEFT OUTER JOIN(right side),RIGHT OUTER JOIN(left side) andFULL OUTER JOIN(both sides), so a downstream rule could remove a group-by key that is not actually functionally determined and change the query result.This PR fixes the FD derivation on join outputs:
computeFd()inLogicalJoinandPhysicalHashJoinis rewritten with an explicit switch over join types:(NULL, NULL)null-extension of unmatched rows;DataTrait.Builder.addFuncDepsDGForOuterJoinNullableSide()/FuncDepsDG.Builder.addDepsForOuterJoinNullableSide()implements the NOT-NULL-determinant filter.nullableflag (e.g. afterLogicalSubQueryAliasToLogicalProjectinlining), so a determinant that became nullable in the immediate child is dropped.Tests in
FdTestare updated (FOJ/LOJ/ROJ no longer propagate nullable-side FDs, while NOT-NULL-determinant FDs from the nullable side are kept), and a newtestNestedOuterJoinNullableDeterminantcovers the nested outer-join case where the determinant's stale non-nullable flag must not leak through, verified on both the logical and the physical join paths.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)