morrySnow commented on code in PR #64618:
URL: https://github.com/apache/doris/pull/64618#discussion_r3457723595
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/InferSetOperatorDistinct.java:
##########
@@ -38,28 +44,137 @@
* </pre>
*/
public class InferSetOperatorDistinct extends OneRewriteRuleFactory {
+ private static final double LOWER_AGGREGATE_EFFECT_COEFFICIENT = 10000;
+ private static final double LOW_AGGREGATE_EFFECT_COEFFICIENT = 1000;
+ private static final double MEDIUM_AGGREGATE_EFFECT_COEFFICIENT = 100;
+
+ private final StatsDerive derive = new StatsDerive(false);
+
@Override
public Rule build() {
return logicalSetOperation()
.when(operation -> operation.getQualifier() ==
Qualifier.DISTINCT)
.then(setOperation -> {
- if (setOperation.children().stream().anyMatch(child ->
child instanceof LogicalAggregate)) {
- return null;
+ ImmutableList.Builder<Plan> newChildren =
+
ImmutableList.builderWithExpectedSize(setOperation.arity());
+ boolean hasNewChildren = false;
+ for (Plan child : setOperation.children()) {
+ if (shouldInferDistinct(child)) {
+ newChildren.add(new LogicalAggregate<>(
+ ImmutableList.copyOf(child.getOutput()),
true, child));
+ hasNewChildren = true;
+ } else {
+ newChildren.add(child);
+ }
}
-
- List<Plan> newChildren = setOperation.children().stream()
- .map(child -> isAgg(child) ? child
- : new
LogicalAggregate<>(ImmutableList.copyOf(child.getOutput()), true, child))
- .collect(ImmutableList.toImmutableList());
- if (newChildren.equals(setOperation.children())) {
+ if (!hasNewChildren) {
return null;
}
- return setOperation.withChildren(newChildren);
+ return setOperation.withChildren(newChildren.build());
}).toRule(RuleType.INFER_SET_OPERATOR_DISTINCT);
}
+ private boolean shouldInferDistinct(Plan child) {
+ return !isAgg(child) && rejectNLJ(child)
+ && shouldGenerateAggregateByNdv(child, child.getOutput());
+ }
+
private boolean isAgg(Plan plan) {
return plan instanceof LogicalAggregate || (plan instanceof
LogicalProject && plan.child(
0) instanceof LogicalAggregate);
}
+
+ // if children exist NLJ, we can't infer distinct
+ // because NLJ could generate bitmap runtime filter. and it will execute
failed when we do infer distinct.
+ private boolean rejectNLJ(Plan plan) {
+ if (plan instanceof LogicalProject) {
+ plan = plan.child(0);
+ }
+ if (plan instanceof LogicalJoin) {
+ LogicalJoin<?, ?> join = (LogicalJoin<?, ?>) plan;
+ return join.getOtherJoinConjuncts().isEmpty();
Review Comment:
This is not equivalent to rejecting NLJ. `LogicalJoinToNestedLoopJoin` uses
`JoinUtils.shouldNestedLoopJoin(join)`, which is based on whether hash join
conjuncts are empty. A hash join such as `t1.id = t2.id AND t1.score >
t2.score` has non-empty `otherJoinConjuncts` but is still a `PhysicalHashJoin`,
so this guard skips a beneficial local distinct even when NDV says it should be
added. Conversely, a cross join has empty `otherJoinConjuncts` and will pass
this guard even though it is an NLJ. Please use the same NLJ predicate as the
implementation rule (or check `hashJoinConjuncts`) instead of using residual
predicates as a proxy.
--
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]
