github-actions[bot] commented on code in PR #65250:
URL: https://github.com/apache/doris/pull/65250#discussion_r3655306003
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/InferAggNotNull.java:
##########
@@ -108,8 +108,7 @@ private Set<Expression> inferFunctionNotNulls(
private boolean canInferFunctionNotNull(AggregateFunction
aggregateFunction) {
return isSupportedAggregateFunction(aggregateFunction)
- && !aggregateFunction.children().isEmpty()
- &&
ExpressionUtils.isCheapEnoughToInferNotNull(aggregateFunction.children());
+ && !aggregateFunction.children().isEmpty();
Review Comment:
[P2] Cover the new partial aggregate-inference boundary
Removing the all-children cheapness gate makes a normalized `COUNT(DISTINCT
c1, ..., c33)` take a new path: `NormalizeAggregate` leaves these arguments as
slots, `mergeInputSlotsWithinLimit` accepts a subset up to the 32-slot cap, and
`InferAggNotNull` now inserts partial `IS NOT NULL` filters where it previously
inserted none. The existing aggregate tests only use unary arguments, so they
do not exercise the accepted/skipped boundary, its common-set intersection with
another aggregate, or NULL-row results. Please add a focused
full-rewrite/result test with more than 32 nullable DISTINCT-count arguments to
lock down this newly reachable contract.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateOuterJoin.java:
##########
@@ -50,28 +47,26 @@ public class EliminateOuterJoin extends
OneRewriteRuleFactory {
public Rule build() {
return logicalFilter(
logicalJoin().when(join -> join.getJoinType().isOuterJoin() ||
join.getJoinType().isAsofOuterJoin())
- ).then(filter -> {
- LogicalJoin<Plan, Plan> join = filter.child();
+ ).thenApply(ctx -> {
+ LogicalJoin<Plan, Plan> join = ctx.root.child();
- Builder<Expression> conjunctsBuilder = ImmutableSet.builder();
- Set<Slot> notNullSlots = new HashSet<>();
- for (Expression predicate : filter.getConjuncts()) {
- Optional<Slot> notNullSlot = TypeUtils.isNotNull(predicate);
- if (notNullSlot.isPresent()) {
- notNullSlots.add(notNullSlot.get());
- } else {
- conjunctsBuilder.add(predicate);
- }
- }
+ Set<Slot> notNullSlots = ExpressionUtils.inferNotNullSlots(
Review Comment:
[P1] Keep evaluation-sensitive filters above the outer join
With `disable_nereids_rules='INFER_FILTER_NOT_NULL'`, this call can change
which rows reach filter expressions. For `Filter(R.b > 0, random(1) > 0.5) ->
LEFT_OUTER_JOIN(L, R)`, the old rule keeps the join outer, while the new
deterministic first conjunct proves `R.b` non-null and makes it INNER. Merely
ignoring the volatile conjunct during inference is insufficient: BE evaluates
every conjunct over the full input block, so unmatched rows advance seeded
`random` only in the outer plan and shift matched-row values. Likewise, `R.flag
= assert_true(L.x > 0, 'boom')` folds to NULL after replacing `R.flag` and can
drop an unmatched row before the required error. Existing rules treat volatile
and `NoneMovableFunction` expressions specially. Please do not change the join
type when any filter conjunct contains either category (or make the shared
helper reject the whole predicate set), and add result/error regressions with
the earlier inference rule disabled.
--
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]
