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/06/17 17:53:59 UTC
[GitHub] [doris] 924060929 opened a new pull request, #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
924060929 opened a new pull request, #10241:
URL: https://github.com/apache/doris/pull/10241
# Proposed changes
Issue Number: close #xxx
## Problem Summary:
Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
## Checklist(Required)
1. Does it affect the original behavior: No
2. Has unit tests been added: Yes
3. Has document been added or modified: No Need
4. Does it need to update dependencies: No
5. Are there any changes that cannot be rolled back: No
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901273524
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -54,11 +56,14 @@ public Group getRoot() {
* @param rewrite whether to rewrite the node to the target group
* @return Reference of node in Memo
*/
- public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
- Preconditions.checkArgument(!rewrite || target != null);
Review Comment:
this line will cause a bug: rewrite could create multi hierarchy, for example new LogicalFilter(new LogicalRelation). the child(LogicalRelation) need be rewrite(insert or update into memo), bug target Group not exist.
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901272091
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteBottomUpJob.java:
##########
@@ -72,12 +72,14 @@ public void execute() throws AnalysisException {
Preconditions.checkArgument(afters.size() == 1);
Plan after = afters.get(0);
if (after != before) {
- context.getOptimizerContext().getMemo().copyIn(after, group, rule.isRewrite());
+ GroupExpression gexpr = context.getOptimizerContext()
+ .getMemo()
+ .copyIn(after, group, rule.isRewrite());
+ gexpr.setApplied(rule);
pushTask(new RewriteBottomUpJob(group, rules, context, false));
return;
}
}
- logicalExpression.setApplied(rule);
Review Comment:
At present, rewrite rule always create new GroupExpression with assign false to applied, So this line is not working.
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901272091
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteBottomUpJob.java:
##########
@@ -72,12 +72,14 @@ public void execute() throws AnalysisException {
Preconditions.checkArgument(afters.size() == 1);
Plan after = afters.get(0);
if (after != before) {
- context.getOptimizerContext().getMemo().copyIn(after, group, rule.isRewrite());
+ GroupExpression gexpr = context.getOptimizerContext()
+ .getMemo()
+ .copyIn(after, group, rule.isRewrite());
+ gexpr.setApplied(rule);
pushTask(new RewriteBottomUpJob(group, rules, context, false));
return;
}
}
- logicalExpression.setApplied(rule);
Review Comment:
this line will cause a dead loop, because rewrite always create new GroupExpression with applied = false
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901287014
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
this condition is prevent the top level pattern is multi/group/multiGroup, because the performance. so we must wrap a top concrete pattern, e.g. union(multi()).
multi/multiGroup will translate to any/group in the matchingChildGroup
```java
// translate MULTI and MULTI_GROUP to ANY and GROUP
if (isLastPattern) {
if (childPattern.isMulti()) {
childPattern = Pattern.ANY;
} else if (childPattern.isMultiGroup()) {
childPattern = Pattern.GROUP;
}
}
```
this is the example for multi and multiGroup:
the pattern: `union(logicalRelation(), multi())` means union with first child is logicalRelation and has empty or multi other children. so this plan trees can be matched and returned:
1. Union(LogicalRelation())
2. Union(LogicalRelation(), LogicalProject()),
3. Union(LogicalProject(), LogicalProject(), LogicalProject()).
and rule can see the real child plan type is logicalRelation and logicalProject.
if the pattern is `union(logicalRelation(), multiGroup())`, the returned corresponding plan trees is
1. Union(LogicalRelation())
2. Union(LogicalRelation(), GroupPlan()),
3. Union(LogicalRelation(), GroupPlan(), GroupPlan())
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901277729
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -18,18 +18,22 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.exceptions.UnboundException;
-import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
import org.apache.doris.nereids.trees.AbstractTreeNode;
import org.apache.doris.nereids.trees.NodeType;
import org.apache.doris.nereids.types.DataType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
import java.util.List;
/**
* Abstract class for all Expression in Nereids.
*/
public abstract class Expression extends AbstractTreeNode<Expression> {
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
Review Comment:
static logger can not save the real child class instance for log.
e.g.
```java
logger.warn("accept() is not implemented by " + this.getClass()); // pay attention to the 'this'
```
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901276488
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalProject.java:
##########
@@ -53,4 +55,9 @@ public String toString() {
public <R, C> R accept(PlanOperatorVisitor<R, C> visitor, Plan plan, C context) {
return visitor.visitPhysicalProject(((PhysicalUnaryPlan<PhysicalProject, Plan>) plan), context);
}
+
+ @Override
+ public List<Expression> getExpressions() {
+ return ImmutableList.of();
Review Comment:
not implement. this PR is a part of analyze pr. next PR will complete for SSB.
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901287014
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
just for the future.
for example. the pattern: `union(logicalRelation(), multi())` means union with first child is logicalRelation and has empty or multi other children. so this plan tree can matched:
1. union(logicalRelation())
2. union(logicalRelation(), logicalProject()),
3. union(logicalRelation(), logicalProject(), logicalProject()).
and rule can see the real child plan type is logicalRelation and logicalProject.
--
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] morningman merged pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
morningman merged PR #10241:
URL: https://github.com/apache/doris/pull/10241
--
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] morrySnow commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901232711
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteBottomUpJob.java:
##########
@@ -72,12 +72,14 @@ public void execute() throws AnalysisException {
Preconditions.checkArgument(afters.size() == 1);
Plan after = afters.get(0);
if (after != before) {
- context.getOptimizerContext().getMemo().copyIn(after, group, rule.isRewrite());
+ GroupExpression gexpr = context.getOptimizerContext()
+ .getMemo()
+ .copyIn(after, group, rule.isRewrite());
+ gexpr.setApplied(rule);
pushTask(new RewriteBottomUpJob(group, rules, context, false));
return;
}
}
- logicalExpression.setApplied(rule);
Review Comment:
this should not be removed
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/exceptions/AnalysisException.java:
##########
@@ -0,0 +1,32 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.exceptions;
+
+/** Nereids's AnalysisException. */
+public class AnalysisException extends RuntimeException {
Review Comment:
ππ»
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -18,18 +18,22 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.exceptions.UnboundException;
-import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
import org.apache.doris.nereids.trees.AbstractTreeNode;
import org.apache.doris.nereids.trees.NodeType;
import org.apache.doris.nereids.types.DataType;
+import org.slf4j.Logger;
Review Comment:
most of doris fe's class use log4j2 directly
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/physical/PhysicalProject.java:
##########
@@ -53,4 +55,9 @@ public String toString() {
public <R, C> R accept(PlanOperatorVisitor<R, C> visitor, Plan plan, C context) {
return visitor.visitPhysicalProject(((PhysicalUnaryPlan<PhysicalProject, Plan>) plan), context);
}
+
+ @Override
+ public List<Expression> getExpressions() {
+ return ImmutableList.of();
Review Comment:
why not return projects?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -54,11 +56,14 @@ public Group getRoot() {
* @param rewrite whether to rewrite the node to the target group
* @return Reference of node in Memo
*/
- public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
- Preconditions.checkArgument(!rewrite || target != null);
Review Comment:
why remove this line?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -69,6 +74,20 @@ public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
// TODO: need to derive logical property if generate new group. currently we not copy logical plan into
}
+ public Plan copyOut() {
Review Comment:
Add a comment stating that this function is only used for testing
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
where do we do multi match? or multi should not return?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -18,18 +18,22 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.exceptions.UnboundException;
-import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
import org.apache.doris.nereids.trees.AbstractTreeNode;
import org.apache.doris.nereids.trees.NodeType;
import org.apache.doris.nereids.types.DataType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
import java.util.List;
/**
* Abstract class for all Expression in Nereids.
*/
public abstract class Expression extends AbstractTreeNode<Expression> {
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
Review Comment:
should be static?
--
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] morrySnow commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901309353
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -54,11 +56,14 @@ public Group getRoot() {
* @param rewrite whether to rewrite the node to the target group
* @return Reference of node in Memo
*/
- public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
- Preconditions.checkArgument(!rewrite || target != null);
Review Comment:
got 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 commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901362112
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -69,6 +74,20 @@ public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
// TODO: need to derive logical property if generate new group. currently we not copy logical plan into
}
+ public Plan copyOut() {
Review Comment:
ok, keep this function for a while, if just for testing, we will comment 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 commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901287014
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
this condition is prevent the top level pattern is multi/group/multiGroup, because of performance. so we must wrap a top concrete pattern, e.g. union(multi()).
multi/multiGroup will translate to any/group in the matchingChildGroup
```java
// translate MULTI and MULTI_GROUP to ANY and GROUP
if (isLastPattern) {
if (childPattern.isMulti()) {
childPattern = Pattern.ANY;
} else if (childPattern.isMultiGroup()) {
childPattern = Pattern.GROUP;
}
}
```
this is the example for multi and multiGroup:
the pattern: `union(logicalRelation(), multi())` means union with first child is logicalRelation and has empty or multi other children. so this plan trees can be matched and returned:
1. Union(LogicalRelation())
2. Union(LogicalRelation(), LogicalProject()),
3. Union(LogicalProject(), LogicalProject(), LogicalProject()).
and rule can see the real child plan type is logicalRelation and logicalProject.
if the pattern is `union(logicalRelation(), multiGroup())`, the returned corresponding plan trees is
1. Union(LogicalRelation())
2. Union(LogicalRelation(), GroupPlan()),
3. Union(LogicalRelation(), GroupPlan(), GroupPlan())
--
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] morrySnow commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901310316
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -69,6 +74,20 @@ public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
// TODO: need to derive logical property if generate new group. currently we not copy logical plan into
}
+ public Plan copyOut() {
Review Comment:
> enerate physical plan tree and translate to thrift struct
copy out could not do that, we need choose best plan to do this work.
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901287014
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
this condition is prevent the top level pattern is multi/group/multiGroup, because of performance. so we must wrap a top concrete pattern, e.g. union(multi()).
multi/multiGroup will translate to any/group in the matchingChildGroup method
```java
// translate MULTI and MULTI_GROUP to ANY and GROUP
if (isLastPattern) {
if (childPattern.isMulti()) {
childPattern = Pattern.ANY;
} else if (childPattern.isMultiGroup()) {
childPattern = Pattern.GROUP;
}
}
```
this is the example for multi and multiGroup:
the pattern: `union(logicalRelation(), multi())` means union with first child is logicalRelation and has empty or multi other children. so this plan trees can be matched and returned:
1. Union(LogicalRelation())
2. Union(LogicalRelation(), LogicalProject()),
3. Union(LogicalRelation(), LogicalProject(), LogicalProject()).
and rule can see the real child plan type is logicalRelation and logicalProject.
if the pattern is `union(logicalRelation(), multiGroup())`, the returned corresponding plan trees is
1. Union(LogicalRelation())
2. Union(LogicalRelation(), GroupPlan()),
3. Union(LogicalRelation(), GroupPlan(), GroupPlan())
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901278864
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -18,18 +18,22 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.exceptions.UnboundException;
-import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
import org.apache.doris.nereids.trees.AbstractTreeNode;
import org.apache.doris.nereids.trees.NodeType;
import org.apache.doris.nereids.types.DataType;
+import org.slf4j.Logger;
Review Comment:
use interface is more flexible and reasonable then a concrete implement
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901287014
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
this condition is prevent the top level pattern is multi/group/multiGroup, because of performance. so we must wrap a top concrete pattern, e.g. union(multi()).
multi/multiGroup will translate to any/group in the matchingChildGroup method
```java
// translate MULTI and MULTI_GROUP to ANY and GROUP
if (isLastPattern) {
if (childPattern.isMulti()) {
childPattern = Pattern.ANY;
} else if (childPattern.isMultiGroup()) {
childPattern = Pattern.GROUP;
}
}
```
this is the example for multi and multiGroup:
the pattern: `union(logicalRelation(), multi())` means union with first child is logicalRelation and has empty or multi other children. so this plan trees can be matched and returned:
1. Union(LogicalRelation())
2. Union(LogicalRelation(), LogicalProject()),
3. Union(LogicalProject(), LogicalProject(), LogicalProject()).
and rule can see the real child plan type is logicalRelation and logicalProject.
if the pattern is `union(logicalRelation(), multiGroup())`, the returned corresponding plan trees is
1. Union(LogicalRelation())
2. Union(LogicalRelation(), GroupPlan()),
3. Union(LogicalRelation(), GroupPlan(), GroupPlan())
--
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] morrySnow commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901309563
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -69,6 +74,20 @@ public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
// TODO: need to derive logical property if generate new group. currently we not copy logical plan into
}
+ public Plan copyOut() {
Review Comment:
π
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901287014
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
just for the future.
for example. the pattern: `union(logicalRelation(), multi())` means union with first child is logicalRelation and has empty or multi other children. so this plan trees can be matched and returned:
1. Union(LogicalRelation())
2. Union(LogicalRelation(), LogicalProject()),
3. Union(LogicalProject(), LogicalProject(), LogicalProject()).
and rule can see the real child plan type is logicalRelation and logicalProject.
if the pattern is `union(logicalRelation(), multiGroup())`, the corresponding plan tree returned is
1. Union(LogicalRelation())
2. Union(LogicalRelation(), GroupPlan()),
3. Union(LogicalRelation(), GroupPlan(), GroupPlan())
--
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] morrySnow commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901308709
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteBottomUpJob.java:
##########
@@ -72,12 +72,14 @@ public void execute() throws AnalysisException {
Preconditions.checkArgument(afters.size() == 1);
Plan after = afters.get(0);
if (after != before) {
- context.getOptimizerContext().getMemo().copyIn(after, group, rule.isRewrite());
+ GroupExpression gexpr = context.getOptimizerContext()
+ .getMemo()
+ .copyIn(after, group, rule.isRewrite());
+ gexpr.setApplied(rule);
pushTask(new RewriteBottomUpJob(group, rules, context, false));
return;
}
}
- logicalExpression.setApplied(rule);
Review Comment:
π
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10241:
URL: https://github.com/apache/doris/pull/10241#issuecomment-1160052477
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] github-actions[bot] commented on pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #10241:
URL: https://github.com/apache/doris/pull/10241#issuecomment-1160076514
PR approved by at least one committer 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] 924060929 commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901362689
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -18,18 +18,22 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.exceptions.UnboundException;
-import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
import org.apache.doris.nereids.trees.AbstractTreeNode;
import org.apache.doris.nereids.trees.NodeType;
import org.apache.doris.nereids.types.DataType;
+import org.slf4j.Logger;
Review Comment:
π, ok
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901277729
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -18,18 +18,22 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.exceptions.UnboundException;
-import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
import org.apache.doris.nereids.trees.AbstractTreeNode;
import org.apache.doris.nereids.trees.NodeType;
import org.apache.doris.nereids.types.DataType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
import java.util.List;
/**
* Abstract class for all Expression in Nereids.
*/
public abstract class Expression extends AbstractTreeNode<Expression> {
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
Review Comment:
static logger can not save the real child class instance for log.
for example, EqualTo is the child class of Expression.
if the logger is static, the log is ' 2022-06-20 14:02:00.123 WARN (Expression) - xxxx'
but we need the concrete class in the log: ' 2022-06-20 14:02:00.123 WARN (EqualTo) - xxxx'
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901280061
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteBottomUpJob.java:
##########
@@ -72,12 +72,14 @@ public void execute() throws AnalysisException {
Preconditions.checkArgument(afters.size() == 1);
Plan after = afters.get(0);
if (after != before) {
- context.getOptimizerContext().getMemo().copyIn(after, group, rule.isRewrite());
+ GroupExpression gexpr = context.getOptimizerContext()
+ .getMemo()
+ .copyIn(after, group, rule.isRewrite());
+ gexpr.setApplied(rule);
pushTask(new RewriteBottomUpJob(group, rules, context, false));
return;
}
}
- logicalExpression.setApplied(rule);
Review Comment:
I think we can fix in the future, when we meet the case that rule apply multi times.
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901287014
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/pattern/GroupExpressionMatching.java:
##########
@@ -60,58 +59,101 @@ public GroupExpressionIterator<Plan> iterator() {
* @param pattern pattern to match
* @param groupExpression group expression to be matched
*/
- public GroupExpressionIterator(Pattern<? extends NODE_TYPE, NODE_TYPE> pattern,
- GroupExpression groupExpression) {
- results = Lists.newArrayList();
-
+ public GroupExpressionIterator(Pattern<Plan, Plan> pattern, GroupExpression groupExpression) {
if (!pattern.matchOperator(groupExpression.getOperator())) {
return;
}
- if (pattern.arity() > groupExpression.arity()) {
+
+ // (logicalFilter(), multi()) match (logicalFilter()),
+ // but (logicalFilter(), logicalFilter(), multi()) not match (logicalFilter())
+ boolean extraMulti = pattern.arity() == groupExpression.arity() + 1
+ && (pattern.hasMultiChild() || pattern.hasMultiGroupChild());
+ if (pattern.arity() > groupExpression.arity() && !extraMulti) {
return;
}
- if (pattern.arity() < groupExpression.arity()
- && (!pattern.children().contains(Pattern.MULTI)
- || !pattern.children().contains(Pattern.MULTI_FIXED))) {
+
+ // (multi()) match (logicalFilter(), logicalFilter()),
+ // but (logicalFilter()) not match (logicalFilter(), logicalFilter())
+ if (!pattern.isAny() && pattern.arity() < groupExpression.arity()
+ && !pattern.hasMultiChild() && !pattern.hasMultiGroupChild()) {
return;
}
- NODE_TYPE root = groupExpression.getOperator().toTreeNode(groupExpression);
-
- List<List<NODE_TYPE>> childrenResults = Lists.newArrayListWithCapacity(groupExpression.arity());
- for (int i = 0; i < groupExpression.arity(); ++i) {
- childrenResults.add(Lists.newArrayList());
- int patternChildIndex = i >= pattern.arity() ? pattern.arity() - 1 : i;
- for (NODE_TYPE child : new GroupMatching<NODE_TYPE>(
- pattern.child(patternChildIndex), groupExpression.child(i))) {
- childrenResults.get(i).add(child);
- }
+ // Pattern.GROUP / Pattern.MULTI / Pattern.MULTI_GROUP can not match GroupExpression
+ if (pattern.isGroup() || pattern.isMulti() || pattern.isMultiGroup()) {
Review Comment:
just for the future.
for example. the pattern: `union(logicalRelation(), multi())` means union with first child is logicalRelation and has empty or multi other children. so this plan trees can be matched and returned:
1. Union(LogicalRelation())
2. Union(LogicalRelation(), LogicalProject()),
3. Union(LogicalProject(), LogicalProject(), LogicalProject()).
and rule can see the real child plan type is logicalRelation and logicalProject.
if the pattern is `union(logicalRelation(), multiGroup())`, the returned corresponding plan trees is
1. Union(LogicalRelation())
2. Union(LogicalRelation(), GroupPlan()),
3. Union(LogicalRelation(), GroupPlan(), GroupPlan())
--
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] morrySnow commented on a diff in pull request #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901311504
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -18,18 +18,22 @@
package org.apache.doris.nereids.trees.expressions;
import org.apache.doris.nereids.exceptions.UnboundException;
-import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
import org.apache.doris.nereids.trees.AbstractTreeNode;
import org.apache.doris.nereids.trees.NodeType;
import org.apache.doris.nereids.types.DataType;
+import org.slf4j.Logger;
Review Comment:
> use interface is more flexible and reasonable then a concrete implement
yes, but fe logger system has some bug, slf4j maybe print out log info into fe.out instead of fe.log.
maybe we need to fix it laterπ
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901272091
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/rewrite/RewriteBottomUpJob.java:
##########
@@ -72,12 +72,14 @@ public void execute() throws AnalysisException {
Preconditions.checkArgument(afters.size() == 1);
Plan after = afters.get(0);
if (after != before) {
- context.getOptimizerContext().getMemo().copyIn(after, group, rule.isRewrite());
+ GroupExpression gexpr = context.getOptimizerContext()
+ .getMemo()
+ .copyIn(after, group, rule.isRewrite());
+ gexpr.setApplied(rule);
pushTask(new RewriteBottomUpJob(group, rules, context, false));
return;
}
}
- logicalExpression.setApplied(rule);
Review Comment:
this line will cause a dead loop, because rewrite aways create new GroupExpression with applied = false
--
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 #10241: [feature](Nereids)Add bind slot reference rules and support any/multi/group/multigroup with predicate pattern
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10241:
URL: https://github.com/apache/doris/pull/10241#discussion_r901275247
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -69,6 +74,20 @@ public GroupExpression copyIn(Plan node, Group target, boolean rewrite) {
// TODO: need to derive logical property if generate new group. currently we not copy logical plan into
}
+ public Plan copyOut() {
Review Comment:
this function is not only for testing, we will use to generate physical plan tree and translate to thrift struct.
--
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