branch-4.1: [fix](fd)drop Function dependencies from join outer side #65982 - #66775
Open
github-actions[bot] wants to merge 1 commit into
Open
branch-4.1: [fix](fd)drop Function dependencies from join outer side #65982#66775github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
### 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 -->
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
|
run buildall |
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.
Cherry-picked from #65982