You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/07/28 09:50:23 UTC
[GitHub] [doris] adonis0147 opened a new pull request, #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
adonis0147 opened a new pull request, #11299:
URL: https://github.com/apache/doris/pull/11299
# Proposed changes
~~Issue Number: close #xxx~~
## Problem Summary:
Rules for normalizing expressions should be applied once before do some extra expression transforms.
## Checklist(Required)
1. Type of your changes:
- [x] Improvement
- [ ] Fix
- [ ] Feature-WIP
- [ ] Feature
- [ ] Doc
- [ ] Refator
- [ ] Others:
2. Does it affect the original behavior:
- [ ] Yes
- [x] No
- [ ] I don't know
3. Has unit tests been added:
- [ ] Yes
- [ ] No
- [x] No Need
4. Has document been added or modified:
- [ ] Yes
- [ ] No
- [x] No Need
5. Does it need to update dependencies:
- [ ] Yes
- [x] No
6. Are there any changes that cannot be rolled back:
- [ ] Yes
- [x] No
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r933724598
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/PushPredicateThroughJoin.java:
##########
@@ -122,15 +121,14 @@ private Plan pushDownPredicate(LogicalJoin<GroupPlan, GroupPlan> joinPlan,
Expression left = ExpressionUtils.and(leftPredicates);
Expression right = ExpressionUtils.and(rightPredicates);
//todo expr should optimize again using expr rewrite
- ExpressionRuleExecutor exprRewriter = new ExpressionRuleExecutor();
Plan leftPlan = joinPlan.left();
Plan rightPlan = joinPlan.right();
if (!left.equals(BooleanLiteral.TRUE)) {
- leftPlan = new LogicalFilter(exprRewriter.rewrite(left), leftPlan);
+ leftPlan = new LogicalFilter(left, leftPlan);
Review Comment:
The original `ExpressionRuleExecutor` mixes two types of rules, ones are used for normalization and the others for optimization. In pushing predicates stage, we don't need to do normalization. This pull request aims at separating these types of rules. If you think we can apply rules to optimize expressions, you can add code to do it.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] 924060929 merged pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
924060929 merged PR #11299:
URL: https://github.com/apache/doris/pull/11299
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] wangshuo128 commented on a diff in pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932127219
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rewrite/OptimizeExpressions.java:
##########
@@ -28,14 +27,13 @@
/**
* optimize expression of plan rule set.
*/
-public class OptimizeExpressionOfPlan extends ExpressionOfPlanRewrite {
+public class OptimizeExpressions extends ExpressionRewrite {
Review Comment:
```suggestion
public class OptimizeExpression extends ExpressionRewrite {
```
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on PR #11299:
URL: https://github.com/apache/doris/pull/11299#issuecomment-1200079020
> Could you explain what kind of normalization is implemented in this PR? It would be good if these informations are added in `Proposed Change` section.
Added.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] 924060929 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r934114530
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/TernaryExpression.java:
##########
@@ -23,17 +23,4 @@
* Interface for all expression that have three children.
*/
public interface TernaryExpression extends TernaryNode<Expression, Expression, Expression, Expression> {
-
-
- default Expression first() {
- return child(0);
- }
-
- default Expression second() {
- return child(1);
- }
-
- default Expression third() {
- return child(2);
- }
}
Review Comment:
The default overwrite function that return expression is necessary, otherwise you will see the TernaryNode return the TreeNode. Remain the default override function in UnaryExpression and BinaryExpression too.
```java
default Expression first() {
return child(0);
}
default Expression second() {
return child(1);
}
default Expression third() {
return child(2);
}
```
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] wangshuo128 commented on a diff in pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932124737
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rewrite/NormalizeExpressions.java:
##########
@@ -27,15 +28,16 @@
/**
* normalize expression of plan rule set.
*/
-public class NormalizeExpressionOfPlan extends ExpressionOfPlanRewrite {
+public class NormalizeExpressions extends ExpressionRewrite {
Review Comment:
```suggestion
public class NormalizeExpression extends ExpressionRewrite {
```
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] wangshuo128 commented on a diff in pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932133075
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/logical/PushDownPredicateTest.java:
##########
@@ -242,6 +243,7 @@ public void pushDownPredicateIntoScanTest4() {
}
private Memo rewrite(Plan plan) {
- return PlanRewriter.topDownRewriteMemo(plan, new ConnectContext(), new PushPredicateThroughJoin());
+ Plan normalizedPlan = PlanRewriter.topDownRewrite(plan, new ConnectContext(), new NormalizeExpressions());
Review Comment:
You could add a new interface in `PlanRewriter` like `topDownRewriteMemo(Plan plan, ConnectContext connectContext, List<List<RuleFactory>> ruleBatches)` to apply rules in batches.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] wangshuo128 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932980621
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/logical/PushDownPredicateTest.java:
##########
@@ -242,6 +243,7 @@ public void pushDownPredicateIntoScanTest4() {
}
private Memo rewrite(Plan plan) {
- return PlanRewriter.topDownRewriteMemo(plan, new ConnectContext(), new PushPredicateThroughJoin());
+ Plan normalizedPlan = PlanRewriter.topDownRewrite(plan, new ConnectContext(), new NormalizeExpressions());
Review Comment:
Ok, never mind.
I was thinking to avoid copying in and copying out of `memo` multiple times.
Let's refactor this when we have more similar cases in the future.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r933724127
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/BinaryExpression.java:
##########
@@ -23,15 +23,4 @@
* Interface for all expression that have two children.
*/
public interface BinaryExpression extends BinaryNode<Expression, Expression, Expression> {
Review Comment:
Like `org.apache.doris.nereids.trees.plans.BinaryPlan`, these interfaces are retained for future.
Because these implementations are the same as `BinaryNode`, I remove those due to the redundancy.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] wangshuo128 commented on pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on PR #11299:
URL: https://github.com/apache/doris/pull/11299#issuecomment-1198052836
I suggest renaming the PR title to "Normalize expressions before performing plan rewriting".
The current title is a bit misleading.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] 924060929 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r934285852
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/TernaryExpression.java:
##########
@@ -23,17 +23,4 @@
* Interface for all expression that have three children.
*/
public interface TernaryExpression extends TernaryNode<Expression, Expression, Expression, Expression> {
-
-
- default Expression first() {
- return child(0);
- }
-
- default Expression second() {
- return child(1);
- }
-
- default Expression third() {
- return child(2);
- }
}
Review Comment:
ok, this problem seems fixed by the refactor generic type, so we can remove this default override functions
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r933724598
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/PushPredicateThroughJoin.java:
##########
@@ -122,15 +121,14 @@ private Plan pushDownPredicate(LogicalJoin<GroupPlan, GroupPlan> joinPlan,
Expression left = ExpressionUtils.and(leftPredicates);
Expression right = ExpressionUtils.and(rightPredicates);
//todo expr should optimize again using expr rewrite
- ExpressionRuleExecutor exprRewriter = new ExpressionRuleExecutor();
Plan leftPlan = joinPlan.left();
Plan rightPlan = joinPlan.right();
if (!left.equals(BooleanLiteral.TRUE)) {
- leftPlan = new LogicalFilter(exprRewriter.rewrite(left), leftPlan);
+ leftPlan = new LogicalFilter(left, leftPlan);
Review Comment:
The original `ExpressionRuleExecutor` mixes two types of rules, ones are used for normalization and the others for optimization. In pushing predicates stage, we don't need to do normalization. This pull request aims at separating these types of rules. If you think we can apply rules to optimize expressions, you are welcome to add code to do it.
BTW, the normalization step is moved to [NereidsPlanner](https://github.com/apache/doris/pull/11299/files#diff-c1663569310d5cae9415d2b9b43196ca09b7761818e456c0b1da9bff5ea75a3c)](https://github.com/apache/doris/pull/11299/files#diff-c1663569310d5cae9415d2b9b43196ca09b7761818e456c0b1da9bff5ea75a3cR124).
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932259213
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/logical/PushDownPredicateTest.java:
##########
@@ -242,6 +243,7 @@ public void pushDownPredicateIntoScanTest4() {
}
private Memo rewrite(Plan plan) {
- return PlanRewriter.topDownRewriteMemo(plan, new ConnectContext(), new PushPredicateThroughJoin());
+ Plan normalizedPlan = PlanRewriter.topDownRewrite(plan, new ConnectContext(), new NormalizeExpressions());
Review Comment:
This code snippet is just for test. Is it necessary to do that?
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] github-actions[bot] commented on pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #11299:
URL: https://github.com/apache/doris/pull/11299#issuecomment-1199069226
PR approved by anyone and no changes requested.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] wangshuo128 commented on a diff in pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932133075
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/logical/PushDownPredicateTest.java:
##########
@@ -242,6 +243,7 @@ public void pushDownPredicateIntoScanTest4() {
}
private Memo rewrite(Plan plan) {
- return PlanRewriter.topDownRewriteMemo(plan, new ConnectContext(), new PushPredicateThroughJoin());
+ Plan normalizedPlan = PlanRewriter.topDownRewrite(plan, new ConnectContext(), new NormalizeExpressions());
Review Comment:
You could add a new interface in `PlanRewriter` like `topDownRewriteMemo(Plan plan, ConnectContext connectContext, List<List<RuleFactory>> ruleBatches)` to run apply rules in batches.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r933724598
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/PushPredicateThroughJoin.java:
##########
@@ -122,15 +121,14 @@ private Plan pushDownPredicate(LogicalJoin<GroupPlan, GroupPlan> joinPlan,
Expression left = ExpressionUtils.and(leftPredicates);
Expression right = ExpressionUtils.and(rightPredicates);
//todo expr should optimize again using expr rewrite
- ExpressionRuleExecutor exprRewriter = new ExpressionRuleExecutor();
Plan leftPlan = joinPlan.left();
Plan rightPlan = joinPlan.right();
if (!left.equals(BooleanLiteral.TRUE)) {
- leftPlan = new LogicalFilter(exprRewriter.rewrite(left), leftPlan);
+ leftPlan = new LogicalFilter(left, leftPlan);
Review Comment:
The original `ExpressionRuleExecutor` mixes two types of rules, ones are used for normalization and the others for optimization. In pushing predicates stage, we don't need to do normalization. This pull request aims at separating these types of rules. If you think we can apply rules to optimize expressions, you are welcome to add code to do it.
BTW, the normalization step is moved to [NereidsPlanner](https://github.com/apache/doris/pull/11299/files#diff-c1663569310d5cae9415d2b9b43196ca09b7761818e456c0b1da9bff5ea75a3cR124). It will be executed before pushing predicates.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] englefly commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
englefly commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r933699230
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/PushPredicateThroughJoin.java:
##########
@@ -122,15 +121,14 @@ private Plan pushDownPredicate(LogicalJoin<GroupPlan, GroupPlan> joinPlan,
Expression left = ExpressionUtils.and(leftPredicates);
Expression right = ExpressionUtils.and(rightPredicates);
//todo expr should optimize again using expr rewrite
- ExpressionRuleExecutor exprRewriter = new ExpressionRuleExecutor();
Plan leftPlan = joinPlan.left();
Plan rightPlan = joinPlan.right();
if (!left.equals(BooleanLiteral.TRUE)) {
- leftPlan = new LogicalFilter(exprRewriter.rewrite(left), leftPlan);
+ leftPlan = new LogicalFilter(left, leftPlan);
Review Comment:
the title is "normalize expression before rewriting", but here we remove normalizing stop before rewriting. Could you explain the reason for this change, please?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/BinaryExpression.java:
##########
@@ -23,15 +23,4 @@
* Interface for all expression that have two children.
*/
public interface BinaryExpression extends BinaryNode<Expression, Expression, Expression> {
Review Comment:
Do we still need this interface if all the functions declarations are removed?
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932257000
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rewrite/NormalizeExpressions.java:
##########
@@ -27,15 +28,16 @@
/**
* normalize expression of plan rule set.
*/
-public class NormalizeExpressionOfPlan extends ExpressionOfPlanRewrite {
+public class NormalizeExpressions extends ExpressionRewrite {
Review Comment:
Renamed.
`NormalizeExpressions` -> `ExpressionNormalization`
`OptimizeExpressions` -> `ExpressionOptimization`
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on pull request #11299: [enhancement](nereids) Don't normalize expressions when pushing down predicates
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on PR #11299:
URL: https://github.com/apache/doris/pull/11299#issuecomment-1198190660
> I suggest renaming the PR title to "Normalize expressions before performing plan rewriting". The current title is a bit misleading.
Good advice.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r933724598
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/PushPredicateThroughJoin.java:
##########
@@ -122,15 +121,14 @@ private Plan pushDownPredicate(LogicalJoin<GroupPlan, GroupPlan> joinPlan,
Expression left = ExpressionUtils.and(leftPredicates);
Expression right = ExpressionUtils.and(rightPredicates);
//todo expr should optimize again using expr rewrite
- ExpressionRuleExecutor exprRewriter = new ExpressionRuleExecutor();
Plan leftPlan = joinPlan.left();
Plan rightPlan = joinPlan.right();
if (!left.equals(BooleanLiteral.TRUE)) {
- leftPlan = new LogicalFilter(exprRewriter.rewrite(left), leftPlan);
+ leftPlan = new LogicalFilter(left, leftPlan);
Review Comment:
The original `ExpressionRuleExecutor` mixes two types of rules, ones are used for normalization and the others for optimization. In pushing predicates stage, we don't need to do normalization. This pull request aims at separating these types of rules. If you think we can apply rules to optimize expressions, you are welcome to add code to do it.
BTW, the normalization step is moved to [NereidsPlanner](https://github.com/apache/doris/pull/11299/files#diff-c1663569310d5cae9415d2b9b43196ca09b7761818e456c0b1da9bff5ea75a3cR124).
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r932901227
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rewrite/OptimizeExpressions.java:
##########
@@ -28,14 +27,13 @@
/**
* optimize expression of plan rule set.
*/
-public class OptimizeExpressionOfPlan extends ExpressionOfPlanRewrite {
+public class OptimizeExpressions extends ExpressionRewrite {
Review Comment:
Renamed
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] englefly commented on pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
englefly commented on PR #11299:
URL: https://github.com/apache/doris/pull/11299#issuecomment-1200049529
Could you explain what kind of normalization is implemented in this PR?
It would be good if these informations are added in `Proposed Change` section.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] 924060929 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r934114530
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/TernaryExpression.java:
##########
@@ -23,17 +23,4 @@
* Interface for all expression that have three children.
*/
public interface TernaryExpression extends TernaryNode<Expression, Expression, Expression, Expression> {
-
-
- default Expression first() {
- return child(0);
- }
-
- default Expression second() {
- return child(1);
- }
-
- default Expression third() {
- return child(2);
- }
}
Review Comment:
The default overwrite function that return expression is necessary, otherwise you will see the TernaryNode.first() return the TreeNode. Remain the default override function in UnaryExpression and BinaryExpression too.
```java
default Expression first() {
return child(0);
}
default Expression second() {
return child(1);
}
default Expression third() {
return child(2);
}
```
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r934132006
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/TernaryExpression.java:
##########
@@ -23,17 +23,4 @@
* Interface for all expression that have three children.
*/
public interface TernaryExpression extends TernaryNode<Expression, Expression, Expression, Expression> {
-
-
- default Expression first() {
- return child(0);
- }
-
- default Expression second() {
- return child(1);
- }
-
- default Expression third() {
- return child(2);
- }
}
Review Comment:
I seems that all classes implement the interface `TernaryExpression` return `Expression` object when the instantiate objects call `first`, `second` and `third` methods. The `CHILD_TYPE` in `TernaryNode` infers to `Expression`.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [doris] adonis0147 commented on a diff in pull request #11299: [enhancement](nereids) Normalize expressions before performing plan rewriting
Posted by GitBox <gi...@apache.org>.
adonis0147 commented on code in PR #11299:
URL: https://github.com/apache/doris/pull/11299#discussion_r934132006
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/TernaryExpression.java:
##########
@@ -23,17 +23,4 @@
* Interface for all expression that have three children.
*/
public interface TernaryExpression extends TernaryNode<Expression, Expression, Expression, Expression> {
-
-
- default Expression first() {
- return child(0);
- }
-
- default Expression second() {
- return child(1);
- }
-
- default Expression third() {
- return child(2);
- }
}
Review Comment:
I seems that all classes implement the interface `TernaryExpression` return `Expression` object when they call `first`, `second` and `third` method. The `CHILD_TYPE` in `TernaryNode` infers to `Expression`.
--
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-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org