github-actions[bot] commented on code in PR #65982:
URL: https://github.com/apache/doris/pull/65982#discussion_r3670745743
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/FuncDepsDG.java:
##########
@@ -219,6 +219,25 @@ public void addDeps(FuncDepsDG funcDepsDG) {
}
}
+ /**
+ * Add FD edges from the nullable side of an outer join. Only keep
edges whose
+ * determinant slots are all NOT NULL in the original child: matched
rows always
+ * carry a non-null determinant, while unmatched rows contribute
(NULL, NULL),
+ * so they cannot collide. Edges with a nullable determinant are
dropped because
+ * a matched row with determinant=NULL could conflict with an
unmatched (NULL, NULL).
+ */
+ public void addDepsForOuterJoinNullableSide(FuncDepsDG funcDepsDG) {
+ for (DGItem dgItem : funcDepsDG.dgItems) {
Review Comment:
[P2] Preserve safe transitive dependencies before filtering direct edges
This loop copies only direct edges. For a real projection chain that
produces `a NOT NULL -> b nullable -> c` while forwarding all three slots, the
graph has no separate `a -> c` edge: this helper keeps `a -> b` and drops `b ->
c`, so recursive FD lookup loses `a -> c`. That dependency is still valid after
null-extension because no original row can have `a=NULL`, while synthetic rows
are all NULL. This makes group/order-key elimination miss a safe reduction.
Please derive/materialize the reachable dependencies for each safe determinant
before rebuilding the filtered graph, and add a nullable-intermediate chain
test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/FuncDepsDG.java:
##########
@@ -219,6 +219,25 @@ public void addDeps(FuncDepsDG funcDepsDG) {
}
}
+ /**
+ * Add FD edges from the nullable side of an outer join. Only keep
edges whose
+ * determinant slots are all NOT NULL in the original child: matched
rows always
+ * carry a non-null determinant, while unmatched rows contribute
(NULL, NULL),
+ * so they cannot collide. Edges with a nullable determinant are
dropped because
+ * a matched row with determinant=NULL could conflict with an
unmatched (NULL, NULL).
+ */
+ public void addDepsForOuterJoinNullableSide(FuncDepsDG funcDepsDG) {
+ for (DGItem dgItem : funcDepsDG.dgItems) {
+ boolean allNotNull = dgItem.slots.stream().allMatch(slot ->
!slot.nullable());
Review Comment:
[P1] Check determinant nullability against the current child output
`dgItem.slots` can retain an older non-nullable `SlotReference` even when
the immediate child now outputs a nullable slot with the same ExprId: slot
equality/hash use only ExprId, and `getOrCreateNode` never replaces the stored
object. A reduced failing tree is:
```
Aggregate(group by r_id, c; output r_id, count(*))
RightOuterJoin
Project(l_id, r_id, coalesce(r_id, 1) AS c)
LeftOuterJoin
Scan L
Scan R(r_id NOT NULL UNIQUE)
Scan V
```
The first outer join/project can emit `(r_id,c)=(NULL,1)`; an unmatched row
from the second join emits `(NULL,NULL)`. This line still sees the historical
non-null `r_id`, keeps `r_id -> c`, and lets `EliminateGroupByKey` drop `c`,
merging two groups. Please canonicalize determinants against the immediate
child's current output/nullability before filtering, and cover this nested
logical and physical path.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJoin.java:
##########
@@ -704,11 +704,46 @@ public void computeEqualSet(Builder builder) {
@Override
public void computeFd(Builder builder) {
- if (!joinType.isLeftSemiOrAntiJoin()) {
- builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
- }
- if (!joinType.isRightSemiOrAntiJoin()) {
- builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
+ switch (joinType) {
+ case INNER_JOIN:
+ case ASOF_LEFT_INNER_JOIN:
+ case ASOF_RIGHT_INNER_JOIN:
+ case CROSS_JOIN:
+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
+
builder.addFuncDepsDG(right().getLogicalProperties().getTrait());
+ break;
+ case LEFT_SEMI_JOIN:
+ case LEFT_ANTI_JOIN:
+ case NULL_AWARE_LEFT_ANTI_JOIN:
+ // Semi/anti joins only output the left side; right-side FDs
are irrelevant.
+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
+ break;
+ case LEFT_OUTER_JOIN:
+ case ASOF_LEFT_OUTER_JOIN:
+ // Left side preserved; right side nullable — keep only FDs
whose
+ // determinant is NOT NULL in the original child.
+
builder.addFuncDepsDG(left().getLogicalProperties().getTrait());
+
builder.addFuncDepsDGForOuterJoinNullableSide(right().getLogicalProperties().getTrait());
Review Comment:
[P2] Preserve FDs proven safe by a null-rejecting join key
Passing only the child trait loses join-domain information. For example, a
right child `Project(r_a, coalesce(r_a, 1) AS r_c)` has the direct FD `r_a ->
r_c` even when `r_a` is nullable. In `L LEFT OUTER JOIN R ON l_k = r_a`,
ordinary equality prevents every `r_a=NULL` child row from matching, those
unmatched right rows are not preserved, and synthetic left-unmatched rows
contribute `(r_a,r_c)=(NULL,NULL)`. Thus the FD remains valid, but this call
drops it based only on schema nullability. Please include nullable-side keys
proven null-rejected by the one-sided join predicate when filtering (without
extending that assumption to full outer or null-safe/unproven predicates), and
mirror it in physical recomputation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
