morrySnow commented on code in PR #64618:
URL: https://github.com/apache/doris/pull/64618#discussion_r3457557089
##########
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.
Review Comment:
**Misleading method name and incomplete NLJ detection**: The method name
`rejectNLJ` reads like an action verb but it returns `true` when it is safe to
proceed (no non-equi join conditions).
1. **Naming**: The method doesn't actually check whether the join is an NLJ
— it checks for non-equi join conjuncts as a proxy. A clearer name would be
`hasNoNonEquiJoinConditions` or `canInferDistinctOnJoin`.
2. **Only peels one level of Project**: If there are nested `LogicalProject`
nodes (e.g., `Project` → `Project` → `Join`), the inner join's
`otherJoinConjuncts` would be missed. Consider recursively unwrapping projects.
--
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]
