github-actions[bot] commented on code in PR #65682:
URL: https://github.com/apache/doris/pull/65682#discussion_r3594102575
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/joinorder/hypergraphv2/HyperGraph.java:
##########
@@ -385,17 +434,44 @@ public boolean addAlias(Alias alias, long subTreeNodes) {
if (bitmap == 0) {
bitmap = subTreeNodes;
addToReplaceMap = false;
- List<NamedExpression> aliasList =
nodeToLiteralAlias.get(bitmap);
+ List<NamedExpression> aliasList =
nodeToProjectedAliases.get(bitmap);
if (aliasList == null) {
aliasList = new ArrayList<>(1);
- nodeToLiteralAlias.put(bitmap, aliasList);
+ nodeToProjectedAliases.put(bitmap, aliasList);
}
aliasList.add(alias);
}
Preconditions.checkArgument(bitmap > 0, "slot must belong to some
table");
slotToHyperNodeMap.put(aliasSlot, bitmap);
+ // Do not add aliases on the nullable side of outer joins to
aliasReplaceMap.
+ // Aliases on the nullable side (e.g., COALESCE(v, 0) AS dv on the
right side of
+ // a LEFT JOIN) must execute BEFORE the outer join's
null-extension.
+ // If added to aliasReplaceMap, they would be unwrapped and
reconstructed above
+ // the outer join by PlanReceiver.proposeProject(), changing
execution order
+ // and producing wrong results.
+ // Instead, add them to nodeToProjectedAliases keyed by
subTreeNodes (the entire
+ // subtree under the source Project), so the alias is only
projected when the full
+ // original subtree is available, preserving the execution
boundary.
+ // Using the minimal referenced bitmap (e.g., {B}) would wrongly
emit the alias at
+ // a leaf level, where an inner nullable-side join could still
null-extend it,
+ // e.g. X LEFT JOIN (Project over A LEFT JOIN B): emitting at {B}
loses the inner
+ // join's null-extension context.
+ // Before storing, resolve chained nullable-side aliases (e.g.
y=x+1 referencing
+ // x=coalesce(...)) via nullableAliasReplaceMap so that all stored
expressions
+ // reference only base columns — avoiding sibling references in a
flat LogicalProject.
+ if (addToReplaceMap && isNullableSide) {
+ alias = (Alias) ExpressionUtils.replaceNameExpression(alias,
nullableAliasReplaceMap);
+ alias = (Alias) ExpressionUtils.replaceNameExpression(alias,
aliasReplaceMap);
+ nullableAliasReplaceMap.put(aliasSlot, alias.child());
Review Comment:
[P2] Preserve the expression-limit fallback
Storing each fully expanded child in this map makes the next chained
replacement grow without the expression-limit fallback used by normal Project
merging. `PlanUtils.tryMergeProjections` keeps separate layers when expansion
raises `EXPRESSION_EXCEEDS_LIMIT`; a valid nullable-side chain of small
Projects such as `x1=B.v+B.v`, `x2=x1+x1`, ... is therefore intentionally left
layered once the combined expression would exceed `expr_children_limit`. This
builder expands the chain again until `Expression.checkLimit` throws, causing
DPHyp graph construction to fail even though normal merging preserves the
layered plan. Preserve those layers (or catch the limit and retain a
materialization boundary), and add a limit-path regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/joinorder/hypergraphv2/HyperGraph.java:
##########
@@ -385,17 +434,44 @@ public boolean addAlias(Alias alias, long subTreeNodes) {
if (bitmap == 0) {
bitmap = subTreeNodes;
addToReplaceMap = false;
- List<NamedExpression> aliasList =
nodeToLiteralAlias.get(bitmap);
+ List<NamedExpression> aliasList =
nodeToProjectedAliases.get(bitmap);
if (aliasList == null) {
aliasList = new ArrayList<>(1);
- nodeToLiteralAlias.put(bitmap, aliasList);
+ nodeToProjectedAliases.put(bitmap, aliasList);
}
aliasList.add(alias);
}
Preconditions.checkArgument(bitmap > 0, "slot must belong to some
table");
slotToHyperNodeMap.put(aliasSlot, bitmap);
+ // Do not add aliases on the nullable side of outer joins to
aliasReplaceMap.
+ // Aliases on the nullable side (e.g., COALESCE(v, 0) AS dv on the
right side of
+ // a LEFT JOIN) must execute BEFORE the outer join's
null-extension.
+ // If added to aliasReplaceMap, they would be unwrapped and
reconstructed above
+ // the outer join by PlanReceiver.proposeProject(), changing
execution order
+ // and producing wrong results.
+ // Instead, add them to nodeToProjectedAliases keyed by
subTreeNodes (the entire
+ // subtree under the source Project), so the alias is only
projected when the full
+ // original subtree is available, preserving the execution
boundary.
+ // Using the minimal referenced bitmap (e.g., {B}) would wrongly
emit the alias at
+ // a leaf level, where an inner nullable-side join could still
null-extend it,
+ // e.g. X LEFT JOIN (Project over A LEFT JOIN B): emitting at {B}
loses the inner
+ // join's null-extension context.
+ // Before storing, resolve chained nullable-side aliases (e.g.
y=x+1 referencing
+ // x=coalesce(...)) via nullableAliasReplaceMap so that all stored
expressions
+ // reference only base columns — avoiding sibling references in a
flat LogicalProject.
+ if (addToReplaceMap && isNullableSide) {
+ alias = (Alias) ExpressionUtils.replaceNameExpression(alias,
nullableAliasReplaceMap);
Review Comment:
[P1] Preserve volatile alias materialization
Unconditionally replacing a chained alias here bypasses the
volatile-expression guard used by normal Project merging. For this nullable
subtree:
```text
Project(x, x AS y)
Project(concat(B.v, uuid()) AS x)
B
```
the original layers evaluate the volatile producer once and guarantee `x =
y`. `PlanUtils.canMergeWithProjections` deliberately rejects flattening this
shape, but this map expands `y` to another `concat(B.v, uuid())` while the
original `x` alias is also retained, so the rebuilt flat Project can return
different UUIDs. This is distinct from the existing boundary and
unresolved-sibling threads: the replacement succeeds but changes values.
Preserve ordered Project layers; when the normal volatile-reference guard
rejects expansion, retain the producer materialization boundary rather than
flattening the aliases, and cover the `x = y` invariant in a nullable-side
regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/joinorder/hypergraphv2/HyperGraph.java:
##########
@@ -385,17 +434,44 @@ public boolean addAlias(Alias alias, long subTreeNodes) {
if (bitmap == 0) {
bitmap = subTreeNodes;
addToReplaceMap = false;
- List<NamedExpression> aliasList =
nodeToLiteralAlias.get(bitmap);
+ List<NamedExpression> aliasList =
nodeToProjectedAliases.get(bitmap);
if (aliasList == null) {
aliasList = new ArrayList<>(1);
- nodeToLiteralAlias.put(bitmap, aliasList);
+ nodeToProjectedAliases.put(bitmap, aliasList);
}
aliasList.add(alias);
}
Preconditions.checkArgument(bitmap > 0, "slot must belong to some
table");
slotToHyperNodeMap.put(aliasSlot, bitmap);
+ // Do not add aliases on the nullable side of outer joins to
aliasReplaceMap.
+ // Aliases on the nullable side (e.g., COALESCE(v, 0) AS dv on the
right side of
+ // a LEFT JOIN) must execute BEFORE the outer join's
null-extension.
+ // If added to aliasReplaceMap, they would be unwrapped and
reconstructed above
+ // the outer join by PlanReceiver.proposeProject(), changing
execution order
+ // and producing wrong results.
+ // Instead, add them to nodeToProjectedAliases keyed by
subTreeNodes (the entire
+ // subtree under the source Project), so the alias is only
projected when the full
+ // original subtree is available, preserving the execution
boundary.
+ // Using the minimal referenced bitmap (e.g., {B}) would wrongly
emit the alias at
+ // a leaf level, where an inner nullable-side join could still
null-extend it,
+ // e.g. X LEFT JOIN (Project over A LEFT JOIN B): emitting at {B}
loses the inner
+ // join's null-extension context.
+ // Before storing, resolve chained nullable-side aliases (e.g.
y=x+1 referencing
+ // x=coalesce(...)) via nullableAliasReplaceMap so that all stored
expressions
+ // reference only base columns — avoiding sibling references in a
flat LogicalProject.
+ if (addToReplaceMap && isNullableSide) {
+ alias = (Alias) ExpressionUtils.replaceNameExpression(alias,
nullableAliasReplaceMap);
+ alias = (Alias) ExpressionUtils.replaceNameExpression(alias,
aliasReplaceMap);
+ nullableAliasReplaceMap.put(aliasSlot, alias.child());
+ List<NamedExpression> aliasList =
nodeToProjectedAliases.get(subTreeNodes);
+ if (aliasList == null) {
+ aliasList = new ArrayList<>(1);
+ nodeToProjectedAliases.put(subTreeNodes, aliasList);
Review Comment:
[P1] Bind consumers to the alias emission bitmap
This stores a nullable alias at its full source subtree, but
`slotToHyperNodeMap` still maps the alias slot only to its referenced inputs. A
reduced failing tree is:
```text
X LEFT JOIN
X
InnerJoin(s = C.k)
Project(s = coalesce(B.v, 0)) over InnerJoin(A, B) // emits at {A,B}
C
```
Here `calculateEndsBySlots` makes the upper predicate a `{B}--{C}` edge.
Both child edges are inner, so conflict rules do not add A and DPHyp can build
`B JOIN C ON s=C.k`; neither child emits `s`, because
`getProjectedAliases(B,B)` requires the full `{A,B}` key, and slot validation
fails. This is distinct from the existing boundary/input-pruning threads: the
consumer runs before the alias exists. Map retained alias slots to
`subTreeNodes` too, or otherwise enforce the source Project boundary, and add a
parent-join-predicate regression.
--
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]
