morrySnow commented on code in PR #52748:
URL: https://github.com/apache/doris/pull/52748#discussion_r2203979424
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateJoinByFK.java:
##########
@@ -79,24 +81,42 @@ public Rule build() {
}
Set<Slot> output = project.getInputSlots();
Set<Slot> foreignKeys = Sets.intersection(foreign.getOutputSet(),
equalSet.getAllItemSet());
- Map<Expression, Expression> outputToForeign =
- tryMapOutputToForeignPlan(foreign, output, equalSet);
+ Map<Slot, Slot> outputToForeign = tryMapOutputToForeignPlan(foreign,
output, equalSet);
if (outputToForeign != null) {
+ Pair<Plan, Set<Slot>> newChildPair =
applyNullCompensationFilter(foreign, foreignKeys);
+ Map<Slot, Expression> replacedSlots =
getReplaceSlotMap(outputToForeign, newChildPair.second);
List<NamedExpression> newProjects = project.getProjects().stream()
- .map(e -> outputToForeign.containsKey(e)
- ? new Alias(e.getExprId(), outputToForeign.get(e),
e.toSql())
- : (NamedExpression) e.rewriteUp(s ->
outputToForeign.getOrDefault(s, s)))
+ .map(e -> replacedSlots.containsKey(e)
+ ? new Alias(e.getExprId(), replacedSlots.get(e),
e.toSql())
+ : (NamedExpression) e.rewriteUp(s ->
replacedSlots.getOrDefault(s, s)))
.collect(ImmutableList.toImmutableList());
- return project.withProjects(newProjects)
- .withChildren(applyNullCompensationFilter(foreign,
foreignKeys));
+ return
project.withProjects(newProjects).withChildren(newChildPair.first);
}
return project;
}
- private @Nullable Map<Expression, Expression>
tryMapOutputToForeignPlan(Plan foreignPlan,
+ // include two types:
+ // primary slot => foreign slot
+ // foreign slot => NonNullable(foreign slot)
+ private Map<Slot, Expression> getReplaceSlotMap(Map<Slot, Slot>
outputToForeign,
Review Comment:
add a ut
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/FillUpMissingSlots.java:
##########
@@ -222,10 +222,14 @@ public List<Rule> buildRules() {
// avoid throw exception even if having have slot
from its child.
// because we will add a project between having
and project.
Resolver resolver = new Resolver(agg, false,
outerScope);
- having.getConjuncts().forEach(resolver::resolve);
+ Set<Expression> adjustAggNullableConjuncts =
having.getConjuncts().stream()
+ .map(conjunct ->
AdjustAggregateNullableForEmptySet.replaceExpression(
+ conjunct, true))
+ .collect(Collectors.toSet());
+
adjustAggNullableConjuncts.forEach(resolver::resolve);
agg =
agg.withAggOutput(resolver.getNewOutputSlots());
Set<Expression> newConjuncts =
ExpressionUtils.replace(
- having.getConjuncts(),
resolver.getSubstitution());
+ adjustAggNullableConjuncts,
resolver.getSubstitution());
Review Comment:
could add some ut for all place changed nullable
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/AdjustAggregateNullableForEmptySet.java:
##########
@@ -43,54 +45,69 @@ public class AdjustAggregateNullableForEmptySet implements
RewriteRuleFactory {
@Override
public List<Rule> buildRules() {
return ImmutableList.of(
- RuleType.ADJUST_NULLABLE_FOR_AGGREGATE_SLOT.build(
- logicalAggregate()
- .then(agg -> {
- List<NamedExpression> outputExprs =
agg.getOutputExpressions();
- boolean noGroupBy =
agg.getGroupByExpressions().isEmpty();
- ImmutableList.Builder<NamedExpression>
newOutput
- =
ImmutableList.builderWithExpectedSize(outputExprs.size());
- for (NamedExpression ne : outputExprs) {
- NamedExpression newExpr =
- ((NamedExpression)
FunctionReplacer.INSTANCE.replace(ne, noGroupBy));
- newOutput.add(newExpr);
- }
- return
agg.withAggOutput(newOutput.build());
- })
- ),
RuleType.ADJUST_NULLABLE_FOR_HAVING_SLOT.build(
logicalHaving(logicalAggregate())
- .then(having -> {
- Set<Expression> conjuncts =
having.getConjuncts();
- boolean noGroupBy =
having.child().getGroupByExpressions().isEmpty();
- ImmutableSet.Builder<Expression>
newConjuncts
- =
ImmutableSet.builderWithExpectedSize(conjuncts.size());
- for (Expression expr : conjuncts) {
- Expression newExpr =
FunctionReplacer.INSTANCE.replace(expr, noGroupBy);
- newConjuncts.add(newExpr);
- }
- return new
LogicalHaving<>(newConjuncts.build(), having.child());
- })
+ .then(having -> replaceHaving(having,
having.child().getGroupByExpressions().isEmpty()))
+ ),
+ RuleType.ADJUST_NULLABLE_FOR_SORT_SLOT.build(
+ logicalSort(logicalAggregate())
+ .then(sort -> replaceSort(sort,
sort.child().getGroupByExpressions().isEmpty()))
+ ),
+ RuleType.ADJUST_NULLABLE_FOR_SORT_SLOT.build(
+ logicalSort(logicalHaving(logicalAggregate()))
+ .then(sort -> replaceSort(sort,
sort.child().child().getGroupByExpressions().isEmpty()))
)
);
}
+ public static Expression replaceExpression(Expression expression, boolean
alwaysNullable) {
+ return FunctionReplacer.INSTANCE.replace(expression, alwaysNullable);
+ }
+
+ private LogicalPlan replaceSort(LogicalSort<?> sort, boolean
alwaysNullable) {
+ List<OrderKey> newOrderKeys = sort.getOrderKeys().stream()
+ .map(key ->
key.withExpression(replaceExpression(key.getExpr(), alwaysNullable)))
+ .collect(ImmutableList.toImmutableList());
Review Comment:
use for loop for better perfermance
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/EliminateJoinByFK.java:
##########
@@ -109,14 +129,16 @@ public Rule build() {
return builder.build();
}
- private Plan applyNullCompensationFilter(Plan child, Set<Slot> childSlots)
{
- Set<Expression> predicates = childSlots.stream()
- .filter(ExpressionTrait::nullable)
- .map(s -> new Not(new IsNull(s)))
- .collect(ImmutableSet.toImmutableSet());
- if (predicates.isEmpty()) {
- return child;
+ private Pair<Plan, Set<Slot>> applyNullCompensationFilter(Plan child,
Set<Slot> childSlots) {
Review Comment:
add a ut?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/stats/ExpressionEstimation.java:
##########
@@ -280,6 +281,13 @@ public ColumnStatistic visitSlotReference(SlotReference
slotReference, Statistic
return context.findColumnStatistics(slotReference);
}
+ @Override
+ public ColumnStatistic visitNonNullable(NonNullable nonNullable,
Statistics context) {
Review Comment:
rebase
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/AdjustNullable.java:
##########
@@ -369,39 +432,53 @@ private <T extends Expression> Optional<T>
updateExpression(T input, Map<ExprId,
// TODO: remove if statement after we ensure be
constant folding do not change
// expr type at all.
changed.set(true);
- return slotReference.withNullableAndDataType(
- replacedSlot.nullable(),
replacedSlot.getDataType()
- );
+ newSlotReference =
slotReference.withNullableAndDataType(
+ replacedSlot.nullable(),
replacedSlot.getDataType());
}
} else if (slotReference.nullable() !=
replacedSlot.nullable()) {
changed.set(true);
- return
slotReference.withNullable(replacedSlot.nullable());
+ newSlotReference =
slotReference.withNullable(replacedSlot.nullable());
}
}
- return slotReference;
+ // for join other conditions, debugCheck = false, for other
case, debugCheck is always true.
+ // Because join other condition use join output's nullable
attribute, outer join may check fail.
+ // At analyzed phase, the slot reference nullable may change,
for example, NormalRepeat may adjust some
+ // slot reference to nullable, after this rule, node above
repeat need adjust.
+ // so analyzed phase don't assert not-nullable -> nullable,
otherwise adjust plan above
+ // repeat may check fail.
+ if (!slotReference.nullable() && newSlotReference.nullable()
+ && !isAnalyzedPhase && debugCheck
+ && ConnectContext.get() != null
+ && ConnectContext.get().getSessionVariable().feDebug) {
+ throw new AnalysisException("AdjustNullable convert slot "
+ slotReference
+ + " from not-nullable to nullable. You can disable
this check by set fe_debug = false.");
+ }
Review Comment:
else, log a warning
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalAggregate.java:
##########
@@ -146,7 +147,11 @@ private LogicalAggregate(
CHILD_TYPE child) {
super(PlanType.LOGICAL_AGGREGATE, groupExpression, logicalProperties,
child);
this.groupByExpressions = ImmutableList.copyOf(groupByExpressions);
- this.outputExpressions = ImmutableList.copyOf(outputExpressions);
+ boolean noGroupby = groupByExpressions.isEmpty();
Review Comment:
need ut
--
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]
