morrySnow commented on code in PR #23935:
URL: https://github.com/apache/doris/pull/23935#discussion_r1316663052
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushdownFilterThroughWindow.java:
##########
Review Comment:
i think we should not only use regression test but also use fe ut to cover
more scenario and ensure all code branch is right
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushdownFilterThroughWindow.java:
##########
@@ -81,88 +55,31 @@ public Rule build() {
return logicalFilter(logicalWindow()).thenApply(ctx -> {
LogicalFilter<LogicalWindow<Plan>> filter = ctx.root;
LogicalWindow<Plan> window = filter.child();
-
- // We have already done such optimization rule, so just ignore it.
- if (window.child(0) instanceof LogicalPartitionTopN) {
- return filter;
- }
-
- List<NamedExpression> windowExprs = window.getWindowExpressions();
- if (windowExprs.size() != 1) {
- return filter;
- }
- NamedExpression windowExpr = windowExprs.get(0);
- if (windowExpr.children().size() != 1 || !(windowExpr.child(0)
instanceof WindowExpression)) {
- return filter;
- }
-
- // Check the filter conditions. Now, we currently only support
simple conditions of the form
- // 'column </ <=/ = constant'. We will extract some related
conjuncts and do some check.
- Set<Expression> conjuncts = filter.getConjuncts();
- Set<Expression> relatedConjuncts =
extractRelatedConjuncts(conjuncts, windowExpr.getExprId());
-
- boolean hasPartitionLimit = false;
- long partitionLimit = Long.MAX_VALUE;
-
- for (Expression conjunct : relatedConjuncts) {
- Preconditions.checkArgument(conjunct instanceof
BinaryOperator);
- BinaryOperator op = (BinaryOperator) conjunct;
- Expression leftChild = op.children().get(0);
- Expression rightChild = op.children().get(1);
-
- Preconditions.checkArgument(leftChild instanceof SlotReference
- && rightChild instanceof IntegerLikeLiteral);
-
- long limitVal = ((IntegerLikeLiteral)
rightChild).getLongValue();
- // Adjust the value for 'limitVal' based on the comparison
operators.
- if (conjunct instanceof LessThan) {
- limitVal--;
+ Set<Expression> commonPartitionKeys =
window.getCommonPartitionKeyFromWindowExpressions();
+ Set<Expression> bottomConjuncts = Sets.newHashSet();
+ Set<Expression> upperConjuncts = Sets.newHashSet();
Review Comment:
use ImmutableSet.Builder to construct set
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalWindow.java:
##########
@@ -224,4 +229,31 @@ public Optional<Plan> pushPartitionLimitThroughWindow(long
partitionLimit, boole
return Optional.ofNullable(window);
}
+
+ /**
+ *
+ * select rank() over (partition by A, B) as r, sum(x) over(A, C) as s
from T;
+ * A is a common partition key for all windowExpressions.
+ * for a common Partition key A, we could push filter A=1 through this
window.
+ */
+ public Set<Expression> getCommonPartitionKeyFromWindowExpressions() {
+ Set<Expression> commonPartitionKeySet = Sets.newHashSet();
Review Comment:
use ImmutableSet.Builder to construct a set
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/RuleSet.java:
##########
@@ -128,14 +129,16 @@ public class RuleSet {
new PushdownFilterThroughAggregation(),
new PushdownFilterThroughRepeat(),
new PushdownFilterThroughSetOperation(),
- new PushdownFilterThroughWindow(),
+ new PushdownPartitionTopNThroughWindow(),
Review Comment:
this name is ambiguity. how about `GeneratePartitionTopNFromWindow`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalWindow.java:
##########
@@ -224,4 +229,31 @@ public Optional<Plan> pushPartitionLimitThroughWindow(long
partitionLimit, boole
return Optional.ofNullable(window);
}
+
+ /**
+ *
+ * select rank() over (partition by A, B) as r, sum(x) over(A, C) as s
from T;
+ * A is a common partition key for all windowExpressions.
+ * for a common Partition key A, we could push filter A=1 through this
window.
+ */
+ public Set<Expression> getCommonPartitionKeyFromWindowExpressions() {
+ Set<Expression> commonPartitionKeySet = Sets.newHashSet();
+ Map<Expression, Integer> partitionKeyCount = Maps.newConcurrentMap();
Review Comment:
why concurrentMap?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/PushdownFilterThroughWindow.java:
##########
@@ -81,88 +55,31 @@ public Rule build() {
return logicalFilter(logicalWindow()).thenApply(ctx -> {
LogicalFilter<LogicalWindow<Plan>> filter = ctx.root;
LogicalWindow<Plan> window = filter.child();
-
- // We have already done such optimization rule, so just ignore it.
- if (window.child(0) instanceof LogicalPartitionTopN) {
- return filter;
- }
-
- List<NamedExpression> windowExprs = window.getWindowExpressions();
- if (windowExprs.size() != 1) {
- return filter;
- }
- NamedExpression windowExpr = windowExprs.get(0);
- if (windowExpr.children().size() != 1 || !(windowExpr.child(0)
instanceof WindowExpression)) {
- return filter;
- }
-
- // Check the filter conditions. Now, we currently only support
simple conditions of the form
- // 'column </ <=/ = constant'. We will extract some related
conjuncts and do some check.
- Set<Expression> conjuncts = filter.getConjuncts();
- Set<Expression> relatedConjuncts =
extractRelatedConjuncts(conjuncts, windowExpr.getExprId());
-
- boolean hasPartitionLimit = false;
- long partitionLimit = Long.MAX_VALUE;
-
- for (Expression conjunct : relatedConjuncts) {
- Preconditions.checkArgument(conjunct instanceof
BinaryOperator);
- BinaryOperator op = (BinaryOperator) conjunct;
- Expression leftChild = op.children().get(0);
- Expression rightChild = op.children().get(1);
-
- Preconditions.checkArgument(leftChild instanceof SlotReference
- && rightChild instanceof IntegerLikeLiteral);
-
- long limitVal = ((IntegerLikeLiteral)
rightChild).getLongValue();
- // Adjust the value for 'limitVal' based on the comparison
operators.
- if (conjunct instanceof LessThan) {
- limitVal--;
+ Set<Expression> commonPartitionKeys =
window.getCommonPartitionKeyFromWindowExpressions();
+ Set<Expression> bottomConjuncts = Sets.newHashSet();
+ Set<Expression> upperConjuncts = Sets.newHashSet();
+ for (Expression expr : filter.getConjuncts()) {
+ boolean pushed = false;
+ for (Expression partitionKey : commonPartitionKeys) {
+ if
(partitionKey.getInputSlots().containsAll(expr.getInputSlots())) {
+ bottomConjuncts.add(expr);
+ pushed = true;
+ break;
+ }
}
- if (limitVal < 0) {
- return new
LogicalEmptyRelation(ctx.statementContext.getNextRelationId(),
filter.getOutput());
- }
- if (hasPartitionLimit) {
- partitionLimit = Math.min(partitionLimit, limitVal);
- } else {
- partitionLimit = limitVal;
- hasPartitionLimit = true;
+ if (!pushed) {
+ upperConjuncts.add(expr);
}
}
-
- if (!hasPartitionLimit) {
- return filter;
+ if (bottomConjuncts.isEmpty()) {
+ return null;
}
- Optional<Plan> newWindow =
window.pushPartitionLimitThroughWindow(partitionLimit, false);
- if (!newWindow.isPresent()) {
- return filter;
- }
- return filter.withChildren(newWindow.get());
+ LogicalFilter bottomFilter = new LogicalFilter(bottomConjuncts,
window.child());
Review Comment:
do not use Raw class
```suggestion
LogicalFilter<Plan> bottomFilter = new
LogicalFilter<>(bottomConjuncts, window.child());
```
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
