morrySnow commented on code in PR #14807:
URL: https://github.com/apache/doris/pull/14807#discussion_r1045252614
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/Validator.java:
##########
@@ -60,7 +62,9 @@ public Plan visitPhysicalFilter(PhysicalFilter<? extends
Plan> filter, CascadesC
// Check filter is from child output.
Set<Slot> childOutputSet = child.getOutputSet();
- Set<Slot> slotsUsedByFilter =
filter.getPredicates().collect(Slot.class::isInstance);
+ Set<Slot> slotsUsedByFilter = filter.getConjuncts().stream()
+ .map(expr -> ((Set<Slot>)
expr.collect(Slot.class::isInstance)))
Review Comment:
this should work
```suggestion
.<Set<Slot>>map(expr ->
expr.collect(Slot.class::isInstance)))
```
##########
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:
why only process the first conjunct?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/PlanUtils.java:
##########
@@ -43,8 +43,7 @@ public static Plan projectOrSelf(List<NamedExpression>
projectExprs, Plan plan)
}
public static Optional<LogicalFilter<? extends Plan>>
filter(List<Expression> predicates, Plan plan) {
- return ExpressionUtils.optionalAnd(predicates)
- .map(predicate -> new LogicalFilter<>(predicate, plan));
+ return ExpressionUtils.optionalAnd(predicates).map(opt -> new
LogicalFilter<>(predicates, plan));
Review Comment:
Try not to make such useless changes unless necessary
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFilter.java:
##########
@@ -41,52 +45,56 @@
* Logical filter plan.
*/
public class LogicalFilter<CHILD_TYPE extends Plan> extends
LogicalUnary<CHILD_TYPE> implements Filter {
- private final Expression predicates;
+ private final List<Expression> conjuncts;
private final boolean singleTableExpressionExtracted;
- public LogicalFilter(Expression predicates, CHILD_TYPE child) {
- this(predicates, Optional.empty(), Optional.empty(), child);
+ public LogicalFilter(List<Expression> conjuncts, CHILD_TYPE child) {
+ this(conjuncts, Optional.empty(), Optional.empty(), child);
}
- public LogicalFilter(Expression predicates,
- boolean singleTableExpressionExtracted,
+ public LogicalFilter(List<Expression> conjuncts, boolean
singleTableExpressionExtracted,
CHILD_TYPE child) {
- this(predicates, Optional.empty(), singleTableExpressionExtracted,
+ this(conjuncts, Optional.empty(), singleTableExpressionExtracted,
Optional.empty(), child);
}
- public LogicalFilter(Expression predicates, Optional<GroupExpression>
groupExpression,
+ public LogicalFilter(List<Expression> conjuncts, Optional<GroupExpression>
groupExpression,
Optional<LogicalProperties> logicalProperties, CHILD_TYPE child) {
- this(predicates, groupExpression, false, logicalProperties, child);
+ this(conjuncts, groupExpression, false, logicalProperties, child);
}
- public LogicalFilter(Expression predicates, Optional<GroupExpression>
groupExpression,
+ public LogicalFilter(List<Expression> conjuncts, Optional<GroupExpression>
groupExpression,
boolean singleTableExpressionExtracted,
Optional<LogicalProperties> logicalProperties, CHILD_TYPE child) {
super(PlanType.LOGICAL_FILTER, groupExpression, logicalProperties,
child);
- this.predicates = Objects.requireNonNull(predicates, "predicates can
not be null");
+ this.conjuncts = Objects.requireNonNull(conjuncts, "conjuncts can not
be null");
Review Comment:
all collector in constructor should use ImmutableXXX.copyOf(...) to init
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/Validator.java:
##########
@@ -50,7 +52,7 @@ 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().get(0) !=
BooleanLiteral.TRUE);
Review Comment:
u should check the size first, and then do get(0)
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFilter.java:
##########
@@ -41,52 +45,56 @@
* Logical filter plan.
*/
public class LogicalFilter<CHILD_TYPE extends Plan> extends
LogicalUnary<CHILD_TYPE> implements Filter {
- private final Expression predicates;
+ private final List<Expression> conjuncts;
Review Comment:
use set instead of list
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/algebra/Filter.java:
##########
@@ -18,17 +18,16 @@
package org.apache.doris.nereids.trees.plans.algebra;
import org.apache.doris.nereids.trees.expressions.Expression;
-import org.apache.doris.nereids.util.ExpressionUtils;
import java.util.List;
/**
* Common interface for logical/physical filter.
*/
public interface Filter {
- Expression getPredicates();
+ List<Expression> getConjuncts();
- default List<Expression> getConjuncts() {
- return ExpressionUtils.extractConjunction(getPredicates());
+ default Expression getPredicate() {
+ return getConjuncts().get(0);
Review Comment:
why return get(0)?, should return
`ExpressionUtils.and(filter.getConjuncts())`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalFilter.java:
##########
@@ -39,35 +41,39 @@
* Physical filter plan.
*/
public class PhysicalFilter<CHILD_TYPE extends Plan> extends
PhysicalUnary<CHILD_TYPE> implements Filter {
+ private final List<Expression> conjuncts;
- private final Expression predicates;
-
- public PhysicalFilter(Expression predicates, LogicalProperties
logicalProperties, CHILD_TYPE child) {
- this(predicates, Optional.empty(), logicalProperties, child);
+ public PhysicalFilter(List<Expression> conjuncts, LogicalProperties
logicalProperties, CHILD_TYPE child) {
+ this(conjuncts, Optional.empty(), logicalProperties, child);
}
- public PhysicalFilter(Expression predicates, Optional<GroupExpression>
groupExpression,
+ public PhysicalFilter(List<Expression> conjuncts,
Optional<GroupExpression> groupExpression,
LogicalProperties logicalProperties, CHILD_TYPE child) {
super(PlanType.PHYSICAL_FILTER, groupExpression, logicalProperties,
child);
- this.predicates = Objects.requireNonNull(predicates, "predicates can
not be null");
+ this.conjuncts = Objects.requireNonNull(conjuncts, "conjuncts can not
be null");
Review Comment:
```suggestion
this.conjuncts =
ImmutableList.copyOf(Objects.requireNonNull(conjuncts, "conjuncts can not be
null"));
```
##########
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().get(0).toSql().equals(expectFilterPredicate))
Review Comment:
get(0) -> ExpressionUtils.and
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/InferPredicates.java:
##########
@@ -89,7 +89,7 @@ public Plan visitLogicalFilter(LogicalFilter<? extends Plan>
filter, JobContext
filter.getConjuncts().forEach(filterPredicates::remove);
if (!filterPredicates.isEmpty()) {
filterPredicates.addAll(filter.getConjuncts());
- return new
LogicalFilter<>(ExpressionUtils.and(Lists.newArrayList(filterPredicates)),
filter.child());
+ return new LogicalFilter<>(Lists.newArrayList(filterPredicates),
filter.child());
Review Comment:
```suggestion
return new
LogicalFilter<>(ImmutableSet.copyOf(filterPredicates), filter.child());
```
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/CheckAnalysisTest.java:
##########
@@ -37,7 +38,7 @@ public class CheckAnalysisTest {
@Test
public void testCheckExpressionInputTypes() {
- Plan plan = new LogicalFilter<>(new And(new IntegerLiteral(1),
BooleanLiteral.TRUE), groupPlan);
+ Plan plan = new LogicalFilter<>(ImmutableList.of(new And(new
IntegerLiteral(1), BooleanLiteral.TRUE)), groupPlan);
Review Comment:
```suggestion
Plan plan = new LogicalFilter<>(ImmutableList.of(new
IntegerLiteral(1), BooleanLiteral.TRUE), groupPlan);
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalFilter.java:
##########
@@ -97,7 +105,7 @@ public List<Slot> computeOutput() {
@Override
public String toString() {
return Utils.toSqlString("LogicalFilter",
- "predicates", predicates
+ "conjuncts", conjuncts
Review Comment:
```suggestion
"predicates", getPredicate()
```
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/analysis/CheckRowPolicyTest.java:
##########
@@ -137,8 +137,8 @@ public void checkOnePolicy() throws Exception {
Assertions.assertTrue(plan instanceof LogicalFilter);
LogicalFilter filter = (LogicalFilter) plan;
Assertions.assertEquals(filter.child(), relation);
- Assertions.assertTrue(filter.getPredicates() instanceof EqualTo);
- Assertions.assertTrue(filter.getPredicates().toString().contains("k1 =
1"));
+ Assertions.assertTrue(filter.getConjuncts().get(0) instanceof EqualTo);
Review Comment:
all get(0) should be replaced by `filter.getPredicate()`, and
`filter.getPredicate` should call `ExpressionUtils.and(getConjuncts())`, please
check by urself
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/logical/PushDownFilterThroughJoinTest.java:
##########
@@ -38,11 +37,13 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
+import java.util.List;
+
/**
* PushdownFilterThroughJoinTest UT.
*/
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
-public class PushdownFilterThroughJoinTest implements PatternMatchSupported {
+public class PushDownFilterThroughJoinTest implements PatternMatchSupported {
Review Comment:
do not change the class name
--
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]
