morrySnow commented on code in PR #14807:
URL: https://github.com/apache/doris/pull/14807#discussion_r1050316030
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/Validator.java:
##########
@@ -50,7 +52,8 @@ public Plan visitPhysicalProject(PhysicalProject<? extends
Plan> project, Cascad
@Override
public Plan visitPhysicalFilter(PhysicalFilter<? extends Plan> filter,
CascadesContext context) {
- Preconditions.checkArgument(filter.getPredicates() !=
BooleanLiteral.TRUE);
+ Preconditions.checkArgument(!filter.getConjuncts().isEmpty()
+ && filter.getExpressions().get(0) != BooleanLiteral.TRUE);
Review Comment:
should check whole predicates != TRUE, not the first conjunct
```
filter.getPredicate() != BooleanLiteral.TRUE
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSlotReference.java:
##########
@@ -254,10 +255,10 @@ public List<Rule> buildRules() {
Set<Slot> boundSlots = Stream.concat(Stream.of(childPlan),
childPlan.children().stream())
.flatMap(plan -> plan.getOutput().stream())
.collect(Collectors.toSet());
- Expression boundPredicates = new SlotBinder(
- toScope(new ArrayList<>(boundSlots)), having,
ctx.cascadesContext
- ).bind(having.getPredicates());
- return new LogicalHaving<>(boundPredicates,
having.child());
+ SlotBinder binder = new
SlotBinder(toScope(Lists.newArrayList(boundSlots)), having,
+ ctx.cascadesContext);
+ Expression boundConjuncts =
binder.bind(having.getPredicate());
Review Comment:
ditto
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSlotReference.java:
##########
@@ -112,9 +113,9 @@ public List<Rule> buildRules() {
RuleType.BINDING_FILTER_SLOT.build(
logicalFilter().when(Plan::canBind).thenApply(ctx -> {
LogicalFilter<GroupPlan> filter = ctx.root;
- Expression boundPredicates = bind(filter.getPredicates(),
filter.children(),
+ Expression boundConjuncts = bind(filter.getPredicate(),
filter.children(),
filter, ctx.cascadesContext);
- return new LogicalFilter<>(boundPredicates,
filter.child());
+ return new LogicalFilter<>(boundConjuncts, filter.child());
})
Review Comment:
this will return 'A and B and C'. u should use
```
Set<Exprssion> boundConjuncts =
filter.getPredicates.stream().map(....).collect(Collectors.toSet()).
```
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/datasets/ssb/SSBJoinReorderTest.java:
##########
@@ -96,7 +96,7 @@ private void test(String sql, List<String>
expectJoinConditions, List<String> ex
for (String expectFilterPredicate : expectFilterPredicates) {
planChecker.matches(
- logicalFilter().when(filter ->
filter.getPredicates().toSql().equals(expectFilterPredicate))
+ logicalFilter().when(filter ->
filter.getConjuncts().iterator().next().toSql().equals(expectFilterPredicate))
Review Comment:
```suggestion
logicalFilter().when(filter ->
filter.getPredicate().toSql().equals(expectFilterPredicate))
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/AnalyzeSubquery.java:
##########
@@ -52,18 +54,18 @@ public List<Rule> buildRules() {
return ImmutableList.of(
RuleType.ANALYZE_FILTER_SUBQUERY.build(
logicalFilter().thenApply(ctx -> {
- LogicalFilter filter = ctx.root;
- Set<SubqueryExpr> subqueryExprs = filter.getPredicates()
+ LogicalFilter<GroupPlan> filter = ctx.root;
+ Set<SubqueryExpr> subqueryExprs =
filter.getExpressions().get(0)
Review Comment:
should collect all subquery expr in all conjuncts, not only in the first one
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/PlanUtils.java:
##########
@@ -42,12 +43,12 @@ public static Plan projectOrSelf(List<NamedExpression>
projectExprs, Plan plan)
return project(projectExprs, plan).map(Plan.class::cast).orElse(plan);
}
- public static Optional<LogicalFilter<? extends Plan>>
filter(List<Expression> predicates, Plan plan) {
- return ExpressionUtils.optionalAnd(predicates)
- .map(predicate -> new LogicalFilter<>(predicate, plan));
+ public static Optional<LogicalFilter<? extends Plan>>
filter(Set<Expression> predicates, Plan plan) {
+ return ExpressionUtils.optionalAnd(predicates.toArray(new
Expression[0]))
+ .map(opt -> new LogicalFilter<>(predicates, plan));
Review Comment:
why must convert to array? u could add a new function that parameter is a
Collection
--
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]
