Skip to content

Commit 99246b4

Browse files
engleflyenglefly
authored
[fix](fd)drop Function dependencies from join outer side (#65982)
### 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 -->
1 parent 9614286 commit 99246b4

5 files changed

Lines changed: 252 additions & 13 deletions

File tree

‎fe/fe-core/src/main/java/org/apache/doris/nereids/properties/DataTrait.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,16 @@ public void addFuncDepsDG(DataTrait fd) {
237237
fdDgBuilder.addDeps(fd.fdDg);
238238
}
239239

240+
/**
241+
* Add FDs from the nullable side of an outer join, filtering out edges whose
242+
* determinant may be NULL in the immediate child's current output — those would
243+
* be invalidated by null-extension of unmatched rows. Determinants are canonicalized
244+
* against childOutput (by ExprId) before the nullability check.
245+
*/
246+
public void addFuncDepsDGForOuterJoinNullableSide(DataTrait fd, List<Slot> childOutput) {
247+
fdDgBuilder.addDepsForOuterJoinNullableSide(fd.fdDg, childOutput);
248+
}
249+
240250
/**add Dependency relation for dominate and dependency*/
241251
public void addDeps(Set<Slot> dominate, Set<Slot> dependency) {
242252
if (dominate.isEmpty() || dependency.isEmpty()) {

‎fe/fe-core/src/main/java/org/apache/doris/nereids/properties/FuncDepsDG.java‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
package org.apache.doris.nereids.properties;
1919

20+
import org.apache.doris.nereids.trees.expressions.ExprId;
2021
import org.apache.doris.nereids.trees.expressions.Slot;
2122

2223
import com.google.common.collect.ImmutableList;
@@ -219,6 +220,43 @@ public void addDeps(FuncDepsDG funcDepsDG) {
219220
}
220221
}
221222

223+
/**
224+
* Add FD edges from the nullable side of an outer join. Only keep edges whose
225+
* determinant slots are all NOT NULL in the immediate child's current output:
226+
* matched rows always carry a non-null determinant, while unmatched rows contribute
227+
* (NULL, NULL), so they cannot collide. Edges with a nullable determinant are dropped
228+
* because a matched row with determinant=NULL could conflict with an unmatched (NULL, NULL).
229+
* The determinant slots stored in the graph may carry a stale nullable flag (slot
230+
* equality/hash use only ExprId and getOrCreateNode never replaces the stored object),
231+
* so each determinant is canonicalized against the child's current output before the
232+
* nullability check.
233+
*/
234+
public void addDepsForOuterJoinNullableSide(FuncDepsDG funcDepsDG, List<Slot> childOutput) {
235+
Map<ExprId, Slot> outputSlotMap = new HashMap<>();
236+
for (Slot slot : childOutput) {
237+
outputSlotMap.put(slot.getExprId(), slot);
238+
}
239+
for (DGItem dgItem : funcDepsDG.dgItems) {
240+
Set<Slot> canonicalSlots = new HashSet<>();
241+
boolean allNotNull = true;
242+
for (Slot slot : dgItem.slots) {
243+
Slot outputSlot = outputSlotMap.get(slot.getExprId());
244+
// a determinant not in the child's output cannot be trusted; drop the edge
245+
if (outputSlot == null || outputSlot.nullable()) {
246+
allNotNull = false;
247+
break;
248+
}
249+
canonicalSlots.add(outputSlot);
250+
}
251+
if (!allNotNull) {
252+
continue;
253+
}
254+
for (int childIdx : dgItem.children) {
255+
addDeps(canonicalSlots, funcDepsDG.dgItems.get(childIdx).slots);
256+
}
257+
}
258+
}
259+
222260
public void replace(Map<Slot, Slot> replaceSlotMap) {
223261
for (DGItem item : dgItems) {
224262
item.replace(replaceSlotMap);

‎fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJoin.java‎

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -720,11 +720,50 @@ public void computeEqualSet(Builder builder) {
720720

721721
@Override
722722
public void computeFd(Builder builder) {
723-
if (!joinType.isLeftSemiOrAntiJoin()) {
724-
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
725-
}
726-
if (!joinType.isRightSemiOrAntiJoin()) {
727-
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
723+
switch (joinType) {
724+
case INNER_JOIN:
725+
case ASOF_LEFT_INNER_JOIN:
726+
case ASOF_RIGHT_INNER_JOIN:
727+
case CROSS_JOIN:
728+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
729+
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
730+
break;
731+
case LEFT_SEMI_JOIN:
732+
case LEFT_ANTI_JOIN:
733+
case NULL_AWARE_LEFT_ANTI_JOIN:
734+
// Semi/anti joins only output the left side; right-side FDs are irrelevant.
735+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
736+
break;
737+
case LEFT_OUTER_JOIN:
738+
case ASOF_LEFT_OUTER_JOIN:
739+
// Left side preserved; right side nullable — keep only FDs whose
740+
// determinant is NOT NULL in the right child's current output.
741+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
742+
builder.addFuncDepsDGForOuterJoinNullableSide(
743+
right().getLogicalProperties().getTrait(), right().getOutput());
744+
break;
745+
case RIGHT_SEMI_JOIN:
746+
case RIGHT_ANTI_JOIN:
747+
// Semi/anti joins only output the right side; left-side FDs are irrelevant.
748+
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
749+
break;
750+
case RIGHT_OUTER_JOIN:
751+
case ASOF_RIGHT_OUTER_JOIN:
752+
// Right side preserved; left side nullable — keep only FDs whose
753+
// determinant is NOT NULL in the left child's current output.
754+
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
755+
builder.addFuncDepsDGForOuterJoinNullableSide(
756+
left().getLogicalProperties().getTrait(), left().getOutput());
757+
break;
758+
case FULL_OUTER_JOIN:
759+
// Both sides are nullable; keep only FDs whose determinant is NOT NULL.
760+
builder.addFuncDepsDGForOuterJoinNullableSide(
761+
left().getLogicalProperties().getTrait(), left().getOutput());
762+
builder.addFuncDepsDGForOuterJoinNullableSide(
763+
right().getLogicalProperties().getTrait(), right().getOutput());
764+
break;
765+
default:
766+
break;
728767
}
729768
}
730769

‎fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalHashJoin.java‎

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -357,11 +357,50 @@ public void computeEqualSet(DataTrait.Builder builder) {
357357

358358
@Override
359359
public void computeFd(DataTrait.Builder builder) {
360-
if (!joinType.isLeftSemiOrAntiJoin()) {
361-
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
362-
}
363-
if (!joinType.isRightSemiOrAntiJoin()) {
364-
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
360+
switch (joinType) {
361+
case INNER_JOIN:
362+
case ASOF_LEFT_INNER_JOIN:
363+
case ASOF_RIGHT_INNER_JOIN:
364+
case CROSS_JOIN:
365+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
366+
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
367+
break;
368+
case LEFT_SEMI_JOIN:
369+
case LEFT_ANTI_JOIN:
370+
case NULL_AWARE_LEFT_ANTI_JOIN:
371+
// Semi/anti joins only output the left side; right-side FDs are irrelevant.
372+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
373+
break;
374+
case LEFT_OUTER_JOIN:
375+
case ASOF_LEFT_OUTER_JOIN:
376+
// Left side preserved; right side nullable — keep only FDs whose
377+
// determinant is NOT NULL in the right child's current output.
378+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
379+
builder.addFuncDepsDGForOuterJoinNullableSide(
380+
right().getLogicalProperties().getTrait(), right().getOutput());
381+
break;
382+
case RIGHT_SEMI_JOIN:
383+
case RIGHT_ANTI_JOIN:
384+
// Semi/anti joins only output the right side; left-side FDs are irrelevant.
385+
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
386+
break;
387+
case RIGHT_OUTER_JOIN:
388+
case ASOF_RIGHT_OUTER_JOIN:
389+
// Right side preserved; left side nullable — keep only FDs whose
390+
// determinant is NOT NULL in the left child's current output.
391+
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
392+
builder.addFuncDepsDGForOuterJoinNullableSide(
393+
left().getLogicalProperties().getTrait(), left().getOutput());
394+
break;
395+
case FULL_OUTER_JOIN:
396+
// Both sides are nullable; keep only FDs whose determinant is NOT NULL.
397+
builder.addFuncDepsDGForOuterJoinNullableSide(
398+
left().getLogicalProperties().getTrait(), left().getOutput());
399+
builder.addFuncDepsDGForOuterJoinNullableSide(
400+
right().getLogicalProperties().getTrait(), right().getOutput());
401+
break;
402+
default:
403+
break;
365404
}
366405
}
367406
}

‎fe/fe-core/src/test/java/org/apache/doris/nereids/properties/FdTest.java‎

Lines changed: 116 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,10 @@
1919

2020
import org.apache.doris.nereids.trees.expressions.Slot;
2121
import org.apache.doris.nereids.trees.plans.Plan;
22+
import org.apache.doris.nereids.trees.plans.logical.LogicalAggregate;
23+
import org.apache.doris.nereids.trees.plans.logical.LogicalJoin;
24+
import org.apache.doris.nereids.trees.plans.physical.PhysicalHashJoin;
25+
import org.apache.doris.nereids.trees.plans.physical.PhysicalPlan;
2226
import org.apache.doris.nereids.util.PlanChecker;
2327
import org.apache.doris.utframe.TestWithFeService;
2428

@@ -27,6 +31,7 @@
2731
import org.junit.jupiter.api.Test;
2832

2933
import java.util.Set;
34+
import java.util.function.Predicate;
3035

3136
class FdTest extends TestWithFeService {
3237
@Override
@@ -46,6 +51,13 @@ protected void runBeforeAll() throws Exception {
4651
+ "UNIQUE KEY(id)\n"
4752
+ "distributed by hash(id) buckets 10\n"
4853
+ "properties('replication_num' = '1');");
54+
createTable("create table test.nullable_uni (\n"
55+
+ "id int,\n"
56+
+ "id2 int not null,\n"
57+
+ "name varchar(128) not null)\n"
58+
+ "UNIQUE KEY(id)\n"
59+
+ "distributed by hash(id) buckets 10\n"
60+
+ "properties('replication_num' = '1');");
4961
connectContext.setDatabase("test");
5062
connectContext.getSessionVariable().setDisableNereidsRules("PRUNE_EMPTY_PARTITION");
5163
}
@@ -147,38 +159,139 @@ void testJoin() {
147159
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
148160
.isDependent(ImmutableSet.of(plan.getOutput().get(1)), ImmutableSet.of(plan.getOutput().get(2))));
149161

150-
// foj
162+
// foj: both sides nullable — keep FDs with NOT NULL determinants
151163
plan = PlanChecker.from(connectContext)
152164
.analyze("select t1.id, t1.id2, t2.id, t2.id2 "
153165
+ "from uni as t1 full outer join uni as t2 on t1.id2 = t2.id2")
154166
.rewrite()
155167
.getPlan();
168+
// t1.id is NOT NULL, so {t1.id} -> {t1.id2} survives null extension
156169
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
157170
.isDependent(ImmutableSet.of(plan.getOutput().get(0)), ImmutableSet.of(plan.getOutput().get(1))));
171+
// t2.id is NOT NULL, so {t2.id} -> {t2.id2} survives null extension
158172
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
159173
.isDependent(ImmutableSet.of(plan.getOutput().get(2)), ImmutableSet.of(plan.getOutput().get(3))));
160174

161-
// loj
175+
// loj: left side preserved, right side nullable — only NOT NULL-determinant FDs from right propagate
162176
plan = PlanChecker.from(connectContext)
163177
.analyze("select t1.id, t1.id2, t2.id, t2.id2 "
164178
+ "from uni as t1 left outer join uni as t2 on t1.id2 = t2.id2")
165179
.rewrite()
166180
.getPlan();
181+
// t1.id is NOT NULL, left side always preserved
167182
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
168183
.isDependent(ImmutableSet.of(plan.getOutput().get(0)), ImmutableSet.of(plan.getOutput().get(1))));
184+
// t2.id is NOT NULL, so {t2.id} -> {t2.id2} survives null extension
169185
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
170186
.isDependent(ImmutableSet.of(plan.getOutput().get(2)), ImmutableSet.of(plan.getOutput().get(3))));
171187

172-
// roj
188+
// roj: right side preserved, left side nullable — only NOT NULL-determinant FDs from left propagate
173189
plan = PlanChecker.from(connectContext)
174190
.analyze("select t1.id, t1.id2, t2.id, t2.id2 "
175191
+ "from uni as t1 right outer join uni as t2 on t1.id2 = t2.id2")
176192
.rewrite()
177193
.getPlan();
194+
// t1.id is NOT NULL, so {t1.id} -> {t1.id2} survives null extension
178195
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
179196
.isDependent(ImmutableSet.of(plan.getOutput().get(0)), ImmutableSet.of(plan.getOutput().get(1))));
197+
// t2.id is NOT NULL, right side always preserved
180198
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
181199
.isDependent(ImmutableSet.of(plan.getOutput().get(2)), ImmutableSet.of(plan.getOutput().get(3))));
200+
201+
// loj with nullable determinant: FD should be dropped
202+
plan = PlanChecker.from(connectContext)
203+
.analyze("select t1.id, t1.id2, t2.id, t2.id2 "
204+
+ "from uni as t1 left outer join nullable_uni as t2 on t1.id2 = t2.id2")
205+
.rewrite()
206+
.getPlan();
207+
// t1 side preserved
208+
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
209+
.isDependent(ImmutableSet.of(plan.getOutput().get(0)), ImmutableSet.of(plan.getOutput().get(1))));
210+
// t2.id is nullable, so {t2.id} -> {t2.id2} should be dropped
211+
Assertions.assertFalse(plan.getLogicalProperties().getTrait()
212+
.isDependent(ImmutableSet.of(plan.getOutput().get(2)), ImmutableSet.of(plan.getOutput().get(3))));
213+
214+
// foj with nullable determinant on one side
215+
plan = PlanChecker.from(connectContext)
216+
.analyze("select t1.id, t1.id2, t2.id, t2.id2 "
217+
+ "from uni as t1 full outer join nullable_uni as t2 on t1.id2 = t2.id2")
218+
.rewrite()
219+
.getPlan();
220+
// t1.id is NOT NULL, so {t1.id} -> {t1.id2} survives
221+
Assertions.assertTrue(plan.getLogicalProperties().getTrait()
222+
.isDependent(ImmutableSet.of(plan.getOutput().get(0)), ImmutableSet.of(plan.getOutput().get(1))));
223+
// t2.id is nullable, so {t2.id} -> {t2.id2} should be dropped
224+
Assertions.assertFalse(plan.getLogicalProperties().getTrait()
225+
.isDependent(ImmutableSet.of(plan.getOutput().get(2)), ImmutableSet.of(plan.getOutput().get(3))));
226+
}
227+
228+
@Test
229+
void testNestedOuterJoinNullableDeterminant() {
230+
// Reduced failing tree from review "Check determinant nullability against the current child output":
231+
// Aggregate(group by r_id, c)
232+
// RightOuterJoin
233+
// Project(l_id, r_id, coalesce(r_id, 1) AS c)
234+
// LeftOuterJoin
235+
// Scan L
236+
// Scan R(r_id NOT NULL UNIQUE)
237+
// Scan V
238+
// r_id is NOT NULL in R but becomes nullable at the inner LOJ output; the Project derives
239+
// r_id -> c from the expression. At the outer join output this FD must be dropped:
240+
// unmatched V rows inject (r_id=NULL, c=NULL), which collides with the Project's own
241+
// (r_id=NULL, c=1). After rewrite the sub-query alias is inlined into a plain project
242+
// (LogicalSubQueryAliasToLogicalProject) whose trait keeps the stale non-nullable r_id,
243+
// so the outer join must still be checked against the immediate child's current output.
244+
// c is kept in the select list so that it is not pruned away before the trait check.
245+
// Disable join reorder to keep the join tree stable (v LEFT OUTER JOIN p as written).
246+
connectContext.getSessionVariable().setDisableJoinReorder(true);
247+
String sql = "select p.id, p.c, count(*) "
248+
+ "from uni as v "
249+
+ "left outer join ("
250+
+ "select l.id2, r.id, coalesce(r.id, 1) as c "
251+
+ "from agg as l left outer join uni as r on l.id2 = r.id2) p "
252+
+ "on v.id2 = p.id2 "
253+
+ "group by p.id, p.c";
254+
255+
LogicalAggregate<?> aggregate = (LogicalAggregate<?>) findNode(
256+
PlanChecker.from(connectContext).analyze(sql).getPlan(), n -> n instanceof LogicalAggregate);
257+
Assertions.assertNotNull(aggregate);
258+
// group by (r_id, c); both are plain slots after subquery inlining
259+
Slot rId = (Slot) aggregate.getGroupByExpressions().get(0);
260+
Slot c = (Slot) aggregate.getGroupByExpressions().get(1);
261+
262+
// logical path: the outer join's trait must not contain r_id -> c
263+
Plan rewritten = PlanChecker.from(connectContext).analyze(sql).rewrite().getPlan();
264+
LogicalJoin<?, ?> outerJoin = (LogicalJoin<?, ?>) findNode(rewritten, n -> n instanceof LogicalJoin);
265+
Assertions.assertNotNull(outerJoin, "rewritten plan: " + rewritten.treeString());
266+
Assertions.assertFalse(outerJoin.getLogicalProperties().getTrait()
267+
.isDependent(ImmutableSet.of(rId), ImmutableSet.of(c)),
268+
"r_id -> c must be dropped at the outer join since r_id is nullable on the outer side");
269+
270+
// physical path: PhysicalHashJoin must drop r_id -> c as well; pick the outer join
271+
// (its subtree contains the inner join). implement() applies the implementation rules
272+
// directly (no CBO), so no table statistics are required.
273+
PhysicalPlan physicalPlan = PlanChecker.from(connectContext)
274+
.analyze(sql).rewrite().implement().getPhysicalPlan();
275+
PhysicalHashJoin<?, ?> physicalOuterJoin = (PhysicalHashJoin<?, ?>) findNode(physicalPlan,
276+
n -> n instanceof PhysicalHashJoin
277+
&& n.anyMatch(p -> p instanceof PhysicalHashJoin && p != n));
278+
Assertions.assertNotNull(physicalOuterJoin, "physical plan: " + physicalPlan.treeString());
279+
Assertions.assertFalse(physicalOuterJoin.getLogicalProperties().getTrait()
280+
.isDependent(ImmutableSet.of(rId), ImmutableSet.of(c)),
281+
"physical join must also drop r_id -> c since r_id is nullable on the outer side");
282+
}
283+
284+
private Plan findNode(Plan plan, Predicate<Plan> predicate) {
285+
if (predicate.test(plan)) {
286+
return plan;
287+
}
288+
for (Plan child : plan.children()) {
289+
Plan found = findNode(child, predicate);
290+
if (found != null) {
291+
return found;
292+
}
293+
}
294+
return null;
182295
}
183296

184297
@Test

0 commit comments

Comments
 (0)