github-actions[bot] commented on code in PR #67889:
URL: https://github.com/apache/doris/pull/67889#discussion_r4002145272
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/RuntimeFilterPushDownVisitor.java:
##########
@@ -341,7 +350,8 @@ private boolean
canPushThroughJoinChild(AbstractPhysicalJoin<? extends Plan, ? e
// A runtime filter is still safe on the null-generating side if
generated NULL rows
// cannot become non-NULL before the parent join condition is
evaluated. For example,
// `b.pk = c.pk` rejects generated NULLs, while `coalesce(b.pk, 0) =
c.pk` may match them.
- return isNullPropagating(ctx.probeExpr);
+ return NullInputEvaluator.evaluateOnFE(ctx.probeExpr,
ctx.probeExpr.getInputSlots(),
Review Comment:
[P1] Preserve the runtime filter origin's NULL-matching semantics
Returning true here assumes the operation that produced the runtime filter
rejects NULL, but `PushDownContext` also represents NULL-aware set-operation
filters. For example:
```text
INTERSECT
Project[S.x] -- runtime values include NULL and 7
Scan S
Project[B.x + CAST(B.x IS NULL AS INT)]
LeftOuterJoin[A.k = B.k]
A: {(1)}
B: {(1,5)}
```
Without pushdown, the second input is `{5}` and the intersection has no
NULL. The set-operation RF rejects 5 at the B scan, however, so the lower join
loses its physical match and manufactures a NULL row; the project still returns
NULL and `INTERSECT` now gains NULL (the analogous `EXCEPT` loses it). The old
structural predicate rejected the nested `IsNull`, while this evaluator admits
it; complex decoupled `<=>` probes have the same missing-origin problem. Please
carry a null-rejecting/null-matching property in `PushDownContext` and require
it before using this NULL proof, or conservatively stop these origins at
null-generating joins. Add a set-operation/outer-join regression for this tree.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/NullInputEvaluator.java:
##########
@@ -0,0 +1,94 @@
+// 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.util;
+
+import org.apache.doris.nereids.rules.expression.ExpressionRewriteContext;
+import org.apache.doris.nereids.rules.expression.rules.FoldConstantRule;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.literal.BooleanLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.Literal;
+import org.apache.doris.nereids.trees.expressions.literal.NullLiteral;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
+/** Evaluate an expression after replacing an exact set of slots with typed
SQL NULLs. */
+public final class NullInputEvaluator {
+
+ /** The only results callers may use to prove behavior for NULL-extended
inputs. */
+ public enum Result {
+ NULL,
+ FALSE,
+ TRUE,
+ OTHER_NON_NULL,
+ UNKNOWN
+ }
+
+ private NullInputEvaluator() {
+ }
+
+ /**
+ * Replace each supplied slot with a NULL of that slot's data type and
constant-fold the
+ * resulting expression. Only a fully folded literal is classified. An
incomplete fold or any
+ * exception is UNKNOWN so optimization callers fail closed.
+ */
+ public static Result evaluate(Expression expression, Set<? extends Slot>
nullSlots,
+ ExpressionRewriteContext context) {
+ return evaluateInternal(expression, nullSlots, context, false);
+ }
+
+ /**
+ * Evaluate using FE constant-folding rules only. Optimizer safety checks
use this entry point
+ * so proving NULL behavior never issues a BE-folding RPC.
+ */
+ public static Result evaluateOnFE(Expression expression, Set<? extends
Slot> nullSlots,
+ ExpressionRewriteContext context) {
+ return evaluateInternal(expression, nullSlots, context, true);
+ }
+
+ private static Result evaluateInternal(Expression expression, Set<?
extends Slot> nullSlots,
+ ExpressionRewriteContext context, boolean feOnly) {
+ try {
+ Map<Expression, Expression> replacements = new HashMap<>();
+ for (Slot slot : nullSlots) {
+ replacements.put(slot, new NullLiteral(slot.getDataType()));
+ }
+ Expression nullInput = ExpressionUtils.replace(expression,
replacements);
+ Expression folded = feOnly
Review Comment:
[P1] Reject non-movable expressions before certifying a NULL fold
This proves the final value after folding, but not that evaluation of the
original expression can be moved. A reduced reachable runtime-filter tree is:
```text
HashJoin[CAST(IF(B.x IS NULL, NULL, assert_true(B.x <> 0, 'bad')) AS INT) =
C.y]
LeftOuterJoin[A.k = B.k]
A: {(1)}
B: {(1,1), (2,0)}
C: {(1)}
```
The original lower join discards `B(2,0)`, so that row never evaluates
`assert_true`. After substituting NULL here, FE folding selects the NULL branch
and returns `Result.NULL`; the runtime-filter caller can then install the
complete expression at the B scan, where `B(2,0)` throws.
`Count(IF(...assert_true...))` reaches the analogous eager-aggregation path and
is evaluated by a pre-aggregate on the discarded base row. The removed
structural/marker checks rejected both shapes. Please fail closed on the
original expression's `NoneMovableFunction` contract before folding (or enforce
that contract in every movement caller), and cover both consumers with
error-preservation tests.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushDownProject.java:
##########
@@ -410,6 +428,23 @@ public <E extends Expression> Optional<E>
pushDownExpression(E expression) {
}
}
+ private boolean canPushDownThroughChild(Expression expression, int
childIndex) {
+ if (!(plan instanceof LogicalJoin)) {
+ return true;
+ }
+ JoinType joinType = ((LogicalJoin<?, ?>) plan).getJoinType();
+ boolean nullGeneratingSide = childIndex == 0
+ ? joinType.isLeftSideNullable() :
joinType.isRightSideNullable();
+ if (!nullGeneratingSide) {
+ return true;
+ }
+ if (!nullInputEvaluationContext.isPresent()) {
+ return false;
+ }
+ return NullInputEvaluator.evaluate(expression,
expression.getInputSlots(),
Review Comment:
[P2] Keep this repeated safety check on FE folding
`NullInputEvaluator.evaluate` enters the full folding pipeline. With
`enable_fold_constant_by_be=true`, replacing the nullable slot makes an
FE-incomplete candidate such as `map_contains_key(r.m, 1)` constant, so
`FoldConstantRuleOnBE` sends a folding RPC and waits up to five seconds. This
predicate is called separately for every eligible `PreferPushDownProject`
expression, allowing N candidates to issue N sequential RPCs even though their
original slot-bearing expressions were not BE-foldable. The PR's other movement
checks use `evaluateOnFE` specifically to fail closed without RPCs; please use
that entry point here too and add a caller-level test with an FE-incomplete
function.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/NullInputEvaluator.java:
##########
@@ -0,0 +1,94 @@
+// 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.util;
+
+import org.apache.doris.nereids.rules.expression.ExpressionRewriteContext;
+import org.apache.doris.nereids.rules.expression.rules.FoldConstantRule;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.expressions.literal.BooleanLiteral;
+import org.apache.doris.nereids.trees.expressions.literal.Literal;
+import org.apache.doris.nereids.trees.expressions.literal.NullLiteral;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+
+/** Evaluate an expression after replacing an exact set of slots with typed
SQL NULLs. */
+public final class NullInputEvaluator {
+
+ /** The only results callers may use to prove behavior for NULL-extended
inputs. */
+ public enum Result {
+ NULL,
+ FALSE,
+ TRUE,
+ OTHER_NON_NULL,
+ UNKNOWN
+ }
+
+ private NullInputEvaluator() {
+ }
+
+ /**
+ * Replace each supplied slot with a NULL of that slot's data type and
constant-fold the
+ * resulting expression. Only a fully folded literal is classified. An
incomplete fold or any
+ * exception is UNKNOWN so optimization callers fail closed.
+ */
+ public static Result evaluate(Expression expression, Set<? extends Slot>
nullSlots,
+ ExpressionRewriteContext context) {
+ return evaluateInternal(expression, nullSlots, context, false);
+ }
+
+ /**
+ * Evaluate using FE constant-folding rules only. Optimizer safety checks
use this entry point
+ * so proving NULL behavior never issues a BE-folding RPC.
+ */
+ public static Result evaluateOnFE(Expression expression, Set<? extends
Slot> nullSlots,
+ ExpressionRewriteContext context) {
+ return evaluateInternal(expression, nullSlots, context, true);
+ }
+
+ private static Result evaluateInternal(Expression expression, Set<?
extends Slot> nullSlots,
+ ExpressionRewriteContext context, boolean feOnly) {
+ try {
+ Map<Expression, Expression> replacements = new HashMap<>();
+ for (Slot slot : nullSlots) {
+ replacements.put(slot, new NullLiteral(slot.getDataType()));
+ }
+ Expression nullInput = ExpressionUtils.replace(expression,
replacements);
+ Expression folded = feOnly
+ ? FoldConstantRule.evaluateOnFE(nullInput, context)
+ : FoldConstantRule.evaluate(nullInput, context);
+ if (!(folded instanceof Literal)) {
+ return Result.UNKNOWN;
+ }
+ if (folded.isNullLiteral()) {
+ return Result.NULL;
+ }
+ if (BooleanLiteral.FALSE.equals(folded)) {
+ return Result.FALSE;
+ }
+ if (BooleanLiteral.TRUE.equals(folded)) {
+ return Result.TRUE;
+ }
+ return Result.OTHER_NON_NULL;
+ } catch (Exception e) {
Review Comment:
[P2] Preserve the FE-debug exception contract
This outer catch also absorbs the exception that
`FoldConstantRule.evaluateInternal` deliberately rethrows when `fe_debug` is
enabled. Before this migration, `ConflictRulesMaker.isEvalToNullOrFalse` and
`ExpressionUtils.matchesWhenSlotIsNull` called `FoldConstantRule.evaluate`
directly, so folding failures surfaced in FE-debug test runs; now both silently
receive `UNKNOWN`. That contradicts the session variable's documented 'throw
exceptions instead [of] swallow them' behavior. Please rethrow here when
`SessionVariable.isFeDebug()` and keep `UNKNOWN` only for ordinary mode, with a
debug-mode exception test.
--
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]
