morrySnow commented on code in PR #64820:
URL: https://github.com/apache/doris/pull/64820#discussion_r3489082962
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggregation.java:
##########
@@ -143,14 +142,18 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, JobConte
if (aggFunction.containsVolatileExpression()) {
return agg;
}
- // CaseWhen and If (which CASE WHEN is normalized into)
must both be checked.
- // When an agg function contains an If/CaseWhen whose
condition tests IS NULL
- // (e.g. count(if(col IS NULL, value, NULL))), pushing it
to the nullable side
- // of an outer join produces wrong results: null-extended
rows make "col IS NULL"
- // TRUE at the top level, but the pre-aggregated count
slot becomes NULL after
- // null-extension, and ifnull(sum(NULL), 0) = 0 instead of
the correct 1.
- if (!hasCaseWhen && aggFunction.anyMatch(e -> e instanceof
CaseWhen || e instanceof If)) {
- hasCaseWhen = true;
+ // NullToNonNullFunction / AlwaysNotNullable: expressions
that can convert NULL
+ // input to non-NULL output (e.g. COALESCE, NVL, IF, CASE
WHEN, Array).
+ // When an agg function contains such an expression
wrapping a column from the
+ // nullable side of an outer join, null-extended rows
would produce non-NULL values
+ // that get counted by the aggregation. But the
pre-aggregation on the base table
+ // cannot see null-extended rows (they are produced by the
join), so the push-down
+ // would lose those contributions — producing wrong
results.
+ if (!containsNullToNonNull
+ && aggFunction.anyMatch(e -> e instanceof
NullToNonNullFunction
+ || (e instanceof AlwaysNotNullable
+ && !((Expression)
e).getInputSlots().isEmpty()))) {
+ containsNullToNonNull = true;
}
if (aggFunction.arity() > 0 && aggFunction.child(0)
instanceof If
Review Comment:
The exact same safety check pattern appears in both this file
(`PushDownAggregation.visitLogicalAggregate`) and in
`EagerAggRewriter.createContextFromProject`:
```java
aggFunction.anyMatch(e -> e instanceof NullToNonNullFunction
|| (e instanceof AlwaysNotNullable
&& !((Expression) e).getInputSlots().isEmpty()))
```
This duplicated logic could drift over time if one site is updated but the
other isn't. Consider extracting it into a static utility method, e.g.:
```java
// In PushDownAggContext or a shared utility
public static boolean containsNullToNonNullExpression(AggregateFunction
aggFunc) {
return aggFunc.anyMatch(e -> e instanceof NullToNonNullFunction
|| (e instanceof AlwaysNotNullable
&& !((Expression) e).getInputSlots().isEmpty()));
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggregation.java:
##########
@@ -265,9 +268,7 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, JobConte
}
LogicalAggregate<Plan> eagerAgg =
agg.withAggOutputChild(newOutputExpressions, child);
- NormalizeAggregate normalizeAggregate = new
NormalizeAggregate();
- return normalizeAggregate.normalizeAgg(eagerAgg,
Optional.empty(),
- context.getCascadesContext());
+ return eagerAgg;
}
} catch (RuntimeException e) {
Review Comment:
The removal of `NormalizeAggregate.normalizeAgg` here means the returned
`eagerAgg` is not re-normalized. While the aggregate was constructed from
already-normalized components (from the original analysis phase), eager
aggregation may reconstruct agg functions through alias substitution via
`createContextFromProject`.
Consider this scenario: the original normalized plan has:
```
Agg(count(#slot))
Project(if(cond, a, b) AS #slot)
```
After `createContextFromProject`, the reconstructed agg function would
contain `if(cond, a, b)` inline instead of `#slot`. The old code would call
`normalizeAgg` which could split this back into a Project + Aggregate. Without
re-normalization, the `if` remains inside the aggregate function.
Please verify this is safe in all cases, or add a test where a Project
contains complex expressions (like `if` or `CaseWhen` with IS NULL conditions)
that `normalizeAgg` would normally split out.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggregation.java:
##########
@@ -189,13 +192,13 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, JobConte
}
}
- groupKeys = groupKeys.stream().distinct().collect(Collectors.toList());
if (!checkSubTreePattern(agg.child())) {
return agg;
}
+ groupKeys = groupKeys.stream().distinct().collect(Collectors.toList());
Review Comment:
The `containsNullToNonNull` field in `PushDownAggContext` is a single global
boolean. Once ANY aggregate function in the batch contains a
`NullToNonNullFunction`, ALL push-downs to the nullable side are blocked — even
for aggregate functions that:
1. Don't contain any `NullToNonNullFunction`, AND
2. Have all their input slots from the preserved side
This is safe (correctness-preserving) but overly conservative. Consider
tracking null-to-non-null presence **per aggregate function** (e.g., a
`Set<AggregateFunction>`) so that aggregate functions without the problematic
pattern can still be pushed down even when other aggregate functions in the
same batch contain `NullToNonNullFunction`.
For example, in `SELECT count(coalesce(t2.col, 0)), sum(t1.val) ... LEFT
JOIN`, `sum(t1.val)` could still be safely pushed to t1 even though
`count(coalesce(t2.col, 0))` cannot be pushed to t2.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/EagerAggRewriter.java:
##########
@@ -250,13 +250,14 @@ private boolean needOutputCountForJoinChild(LogicalJoin<?
extends Plan, ? extend
&& hasAggNeedJoinMultiplicityRecovery(oppositeAggFunctions);
}
- private Pair<Boolean, Boolean> adjustPushSideForCaseWhen(
+ private Pair<Boolean, Boolean> adjustPushSideForNullToNonNull(
LogicalJoin<? extends Plan, ? extends Plan> join,
PushDownAggContext context,
boolean toLeft, boolean toRight) {
- // Do not push aggregation to the nullable side of outer joins when
agg function contains case-when.
- // CaseWhen expressions may produce non-null values from null-padded
rows (e.g., WHEN col IS NULL THEN -54),
+ // Do not push aggregation to the nullable side of outer joins when
agg function contains
+ // a NullToNonNullFunction (e.g. COALESCE, NVL, IF, CASE WHEN,
NULL_OR_EMPTY).
+ // These expressions may produce non-null values from null-padded rows,
// so pre-aggregation before the join loses those contributions.
- if (!(context.hasDecomposedAggIf || context.hasCaseWhen)) {
+ if (!(context.hasDecomposedAggIf || context.containsNullToNonNull)) {
return Pair.of(toLeft, toRight);
Review Comment:
Minor naming inconsistency: **"NULL_OR_EMPTY"** is the SQL function name,
while all other entries in the same list use Java class names: `COALESCE`,
`NVL`, `IF`, `CASE WHEN`. For consistency, this should be `NullOrEmpty` (the
Java class name).
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/PushDownAggregation.java:
##########
@@ -143,14 +142,18 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> agg, JobConte
if (aggFunction.containsVolatileExpression()) {
return agg;
}
- // CaseWhen and If (which CASE WHEN is normalized into)
must both be checked.
- // When an agg function contains an If/CaseWhen whose
condition tests IS NULL
- // (e.g. count(if(col IS NULL, value, NULL))), pushing it
to the nullable side
- // of an outer join produces wrong results: null-extended
rows make "col IS NULL"
- // TRUE at the top level, but the pre-aggregated count
slot becomes NULL after
- // null-extension, and ifnull(sum(NULL), 0) = 0 instead of
the correct 1.
- if (!hasCaseWhen && aggFunction.anyMatch(e -> e instanceof
CaseWhen || e instanceof If)) {
- hasCaseWhen = true;
+ // NullToNonNullFunction / AlwaysNotNullable: expressions
that can convert NULL
+ // input to non-NULL output (e.g. COALESCE, NVL, IF, CASE
WHEN, Array).
+ // When an agg function contains such an expression
wrapping a column from the
+ // nullable side of an outer join, null-extended rows
would produce non-NULL values
+ // that get counted by the aggregation. But the
pre-aggregation on the base table
+ // cannot see null-extended rows (they are produced by the
join), so the push-down
+ // would lose those contributions — producing wrong
results.
Review Comment:
The comment mentions **"Array"** as an example of an expression that can
convert NULL to non-NULL, alongside COALESCE, NVL, IF, and CASE WHEN. However,
`Array` does not implement `NullToNonNullFunction` in this PR.
I verified that `Array`
(`fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/Array.java`)
does implement `AlwaysNotNullable`, and semantically `array(NULL)` → `[NULL]`
(non-NULL array), so it qualifies. Similarly, I found these other
`AlwaysNotNullable` struct constructors that also convert NULL inputs to
non-NULL outputs but are missing `NullToNonNullFunction`:
- `JsonArray`
- `JsonObject`
- `CreateStruct`
- `CreateMap`
- `CreateNamedStruct`
Please either:
1. Add `NullToNonNullFunction` to `Array` (and the other struct
constructors), or
2. Remove "Array" from this comment to avoid confusion about what is covered.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/EagerAggRewriter.java:
##########
@@ -790,21 +793,18 @@ private Plan genAggregate(Plan child, PushDownAggContext
context) {
aggOutputExpressions.add(outputCountAlias.get());
}
LogicalAggregate genAgg = new
LogicalAggregate(context.getGroupKeys(), aggOutputExpressions, child);
- NormalizeAggregate normalizeAggregate = new NormalizeAggregate();
- Plan normalized = normalizeAggregate.normalizeAgg(genAgg,
Optional.empty(),
- context.getCascadesContext());
for (AggregateFunction func : context.getAggFunctions()) {
Alias a = context.getAliasMap().get(func);
Review Comment:
Removing the `NormalizeAggregate.normalizeAgg` call before returning from
`genAggregate`. Same concern as in `PushDownAggregation.visitLogicalAggregate`.
Additionally, note the **slot registration order** change: the old code
registered pushed slots from the `normalized` plan (result of `normalizeAgg`),
while the new code registers from `genAgg` directly. If `normalizeAgg`
previously changed output expressions (e.g., by pushing group-by expressions
into a child Project), the slot lookup at lines 137-143 would find different
slots in the normalized plan vs. the un-normalized one. Please verify this
doesn't cause missing slot registrations in edge cases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/eageraggregation/EagerAggRewriter.java:
##########
@@ -382,21 +383,23 @@ private PushDownAggContext createContextFromProject(
aggFunctions.add(newAggFunc);
}
// After pushing expressions past the project, the agg functions may
now
- // contain If/CaseWhen that were hidden behind slot references before.
- // e.g. count(#slot) where #slot = if(cond, a, b) in the project.
- // We must re-check and update hasCaseWhen accordingly.
- boolean newHasCaseWhen = context.hasCaseWhen;
- if (!newHasCaseWhen) {
+ // contain NullToNonNull expressions that were hidden behind slot
references before.
+ // e.g. count(#slot) where #slot = coalesce(a, 0) in the project.
+ // We must re-check and update containsNullToNonNull accordingly.
+ boolean newContainsNullToNonNull = context.containsNullToNonNull;
+ if (!newContainsNullToNonNull) {
for (AggregateFunction aggFunc : aggFunctions) {
- if (aggFunc.anyMatch(e -> e instanceof CaseWhen || e
instanceof If)) {
- newHasCaseWhen = true;
+ if (aggFunc.anyMatch(e -> e instanceof NullToNonNullFunction
+ || (e instanceof AlwaysNotNullable
+ && !((Expression)
e).getInputSlots().isEmpty()))) {
+ newContainsNullToNonNull = true;
break;
}
Review Comment:
Same duplicated `NullToNonNullFunction || AlwaysNotNullable` check as in
`PushDownAggregation.visitLogicalAggregate`. See the corresponding comment
there about extracting to a shared helper method to prevent drift.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/NullToNonNullFunction.java:
##########
@@ -0,0 +1,33 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.trees.expressions;
+
+/**
+ * Marker interface for expressions that can convert NULL input into a
non-NULL output.
+ *
+ * For example: COALESCE(NULL, 2) → 2, NVL(NULL, 0) → 0, NULL_OR_EMPTY(NULL) →
true.
+ *
+ * This is significant for outer-join push-down safety: when an aggregate
function contains
+ * a NullToNonNull expression wrapping a column from the nullable side of an
outer join,
+ * the aggregation must NOT be pushed down. Null-extended rows (produced by
the join for
+ * unmatched rows) have NULL for all nullable-side columns. The NullToNonNull
expression
+ * would convert those NULLs to non-NULL values, and the pre-aggregation would
miss those
+ * contributions because null-extended rows do not exist in the base table.
+ */
Review Comment:
Consider adding `@see` Javadoc tags linking to the implementing classes.
This helps future developers understand the full set of expressions covered by
this interface and makes it easier to determine whether a new expression should
implement it:
```java
/**
* ...existing docs...
*
* @see CaseWhen
* @see IsNull
* @see NullSafeEqual
* @see Coalesce
* @see Nvl
* @see If
* @see NullOrEmpty
* @see NotNullOrEmpty
* @see Ipv4StringToNumOrDefault
* @see Ipv6StringToNumOrDefault
* @see ToIpv4OrDefault
* @see ToIpv6OrDefault
*/
```
--
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]
