Copilot commented on code in PR #42199:
URL: https://github.com/apache/doris/pull/42199#discussion_r2108620634
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/InferPredicatesTest.java:
##########
@@ -140,62 +142,66 @@ void inferPredicatesTest04() {
.rewrite()
.matches(
logicalJoin(
- logicalFilter(logicalOlapScan()).when(filter ->
!ExpressionUtils.isInferred(filter.getPredicate())
- &
filter.getPredicate().toSql().contains("id IN (1, 2, 3)")),
- logicalFilter(logicalOlapScan()).when(filter ->
ExpressionUtils.isInferred(filter.getPredicate())
- &
filter.getPredicate().toSql().contains("sid IN (1, 2, 3)"))
+ logicalFilter(logicalOlapScan()).when(
+ filter ->
!ExpressionUtils.isInferred(filter.getPredicate())
+ &
filter.getPredicate().toSql().contains("id IN (1, 2, 3)")),
+ logicalFilter(logicalOlapScan()).when(
+ filter ->
ExpressionUtils.isInferred(filter.getPredicate())
+ &
filter.getPredicate().toSql().contains("sid IN (1, 2, 3)"))
)
);
}
@Test
void inferPredicatesTest05() {
- String sql = "select * from student join score on student.id =
score.sid join course on score.sid = course.id where student.id > 1";
+ String sql
Review Comment:
[nitpick] Splitting the SQL string assignment across multiple lines reduces
readability. Consider using Java text blocks (`"""..."""`) for multiline SQL or
keep the entire statement on one line.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpPredicates.java:
##########
@@ -253,8 +264,13 @@ public ImmutableSet<Expression>
visitLogicalJoin(LogicalJoin<? extends Plan, ? e
public ImmutableSet<Expression> visitLogicalProject(LogicalProject<?
extends Plan> project, Void context) {
return cacheOrElse(project, () -> {
ImmutableSet<Expression> childPredicates =
project.child().accept(this, context);
- Set<Expression> allPredicates =
Sets.newLinkedHashSet(childPredicates);
- for (Entry<Slot, Expression> kv :
project.getAliasToProducer().entrySet()) {
+ Set<Expression> allPredicates = Sets.newLinkedHashSet();
Review Comment:
When `getAllPredicates` is false, the original `childPredicates` are never
added, so any predicates not matching an alias replacement are dropped.
Consider initializing `allPredicates` with `childPredicates` or selectively
adding predicates that reference directly projected slots.
```suggestion
Set<Expression> allPredicates =
Sets.newLinkedHashSet(childPredicates);
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PullUpPredicates.java:
##########
@@ -270,28 +286,40 @@ public ImmutableSet<Expression>
visitLogicalProject(LogicalProject<? extends Pla
});
}
+ /* e.g. LogicalAggregate(output:max(a), min(a), avg(a))
+ +--LogicalFilter(a>1, a<10)
+ when a>1 is pulled up, we can have max(a)>1, min(a)>1 and avg(a)>1
+ and a<10 is pulled up, we can have max(a)<10, min(a)<10 and avg(a)<10
+ * */
@Override
public ImmutableSet<Expression> visitLogicalAggregate(LogicalAggregate<?
extends Plan> aggregate, Void context) {
return cacheOrElse(aggregate, () -> {
ImmutableSet<Expression> childPredicates =
aggregate.child().accept(this, context);
- // TODO
List<NamedExpression> outputExpressions =
aggregate.getOutputExpressions();
-
- Map<Expression, Slot> expressionSlotMap
- =
Maps.newLinkedHashMapWithExpectedSize(outputExpressions.size());
+ Map<Expression, List<Slot>> expressionSlotMap
+ =
Maps.newHashMapWithExpectedSize(outputExpressions.size());
for (NamedExpression output : outputExpressions) {
- if (hasAgg(output)) {
- expressionSlotMap.putIfAbsent(
- output instanceof Alias ? output.child(0) :
output, output.toSlot()
- );
+ if (output instanceof Alias &&
supportPullUpAgg(output.child(0))) {
+ expressionSlotMap.computeIfAbsent(output.child(0).child(0),
+ k -> new ArrayList<>()).add(output.toSlot());
+ }
+ }
+ Set<Expression> pullPredicates = new
LinkedHashSet<>(childPredicates);
+ for (Expression childPredicate : childPredicates) {
+ if (childPredicate instanceof ComparisonPredicate) {
+ ComparisonPredicate cmp = (ComparisonPredicate)
childPredicate;
+ if (cmp.left() instanceof SlotReference && cmp.right()
instanceof Literal
Review Comment:
The code only handles comparison predicates with slot on the left and
literal on the right. Predicates like `literal < slot` won’t be inferred.
Consider detecting both operand orders or normalizing predicate direction
before inference.
--
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]
