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/20 03:36:34 UTC
[GitHub] [doris] morrySnow commented on a diff in pull request #10227: [feature](nereids) Add ssb related expressions and PlanNode
morrySnow commented on code in PR #10227:
URL: https://github.com/apache/doris/pull/10227#discussion_r901220284
##########
fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -178,6 +221,12 @@ booleanValue
: TRUE | FALSE
;
+aggFunction
+ : AVG '(' DISTINCT? expression ')'
Review Comment:
Is this a temporary solution? it is better not add any function name here.
If this is a temporary solution, please add a todo comment.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Literal.java:
##########
@@ -109,7 +109,7 @@ public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
@Override
public String sql() {
- return null;
+ return value.toString();
Review Comment:
Should we break them down into separate expressions @EmmyMiao87 ?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/analysis/FunctionParams.java:
##########
@@ -0,0 +1,83 @@
+// 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.trees.analysis;
+
+import org.apache.doris.nereids.trees.expressions.Expression;
+
+import com.google.common.collect.Lists;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * Function's Params.
+ */
+public class FunctionParams {
+ private boolean isStar;
Review Comment:
just curious, why FunctionParams is a top level class, but SortItem is an inner class?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalSort.java:
##########
@@ -0,0 +1,103 @@
+// 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.operators.plans.logical;
+
+import org.apache.doris.nereids.exceptions.UnboundException;
+import org.apache.doris.nereids.operators.OperatorType;
+import org.apache.doris.nereids.trees.expressions.NamedExpression;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.plans.Plan;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ Logical Sort plan operator.
+ */
+public class LogicalSort extends LogicalUnaryOperator {
+
+ private List<SortItems> sortItems;
+
+ /**
+ * Constructor for SortItems.
+ */
+ public LogicalSort(List<SortItems> sortItems) {
+ super(OperatorType.LOGICAL_SORT);
+ this.sortItems = Objects.requireNonNull(sortItems, "sorItems can not be null");
+ }
+
+ @Override
+ public String toString() {
+ return "Sort (" + StringUtils.join(sortItems, ", ") + ")";
+ }
+
+ @Override
+ public List<Slot> computeOutput(Plan input) {
+ return sortItems.stream()
+ .map(namedExpr -> {
+ try {
+ return namedExpr.getSort().toSlot();
+ } catch (UnboundException e) {
+ throw new IllegalStateException(e);
+ }
+ })
+ .collect(ImmutableList.toImmutableList());
Review Comment:
sort node's output should just return child's output, but not use sort item
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/BetweenPredicate.java:
##########
@@ -0,0 +1,85 @@
+// 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.trees.expressions;
+
+import org.apache.doris.nereids.exceptions.UnboundException;
+import org.apache.doris.nereids.rules.expression.rewrite.ExpressionVisitor;
+import org.apache.doris.nereids.trees.NodeType;
+import org.apache.doris.nereids.types.BooleanType;
+import org.apache.doris.nereids.types.DataType;
+
+/**
+ * Between predicate expression.
+ */
+public class BetweenPredicate extends Expression {
+
+ private boolean isNotBetween;
Review Comment:
we should not include NOT here, but use NotExpression with BetweenPredicate as child
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/operators/plans/logical/LogicalAggregation.java:
##########
@@ -0,0 +1,75 @@
+// 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.operators.plans.logical;
+
+import org.apache.doris.nereids.exceptions.UnboundException;
+import org.apache.doris.nereids.operators.OperatorType;
+import org.apache.doris.nereids.trees.expressions.NamedExpression;
+import org.apache.doris.nereids.trees.expressions.Slot;
+import org.apache.doris.nereids.trees.plans.Plan;
+
+import com.google.common.collect.ImmutableList;
+import org.apache.commons.lang3.StringUtils;
+
+import java.util.List;
+import java.util.Objects;
+
+/**
+ * Logical Aggregation plan operator.
+ */
+public class LogicalAggregation extends LogicalUnaryOperator {
+
+ private final List<? extends NamedExpression> aggregation;
Review Comment:
i think we need to add group by expression here
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -162,7 +191,7 @@ public List<Expression> visitNamedExpressionSeq(NamedExpressionSeqContext ctx) {
List<Expression> expressions = Lists.newArrayList();
if (ctx != null) {
for (NamedExpressionContext namedExpressionContext : ctx.namedExpression()) {
- NamedExpression namedExpression = typedVisit(namedExpressionContext);
+ Expression namedExpression = typedVisit(namedExpressionContext);
Review Comment:
👍🏻
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Arithmetic.java:
##########
@@ -0,0 +1,133 @@
+// 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.trees.expressions;
+
+
+import org.apache.doris.nereids.trees.NodeType;
+import org.apache.doris.thrift.TExprOpcode;
+
+/**
+ * All arithmetic operator.
+ */
+public class Arithmetic<LEFT_CHILD_TYPE extends Expression, RIGHT_CHILD_TYPE extends Expression>
+ extends Expression implements BinaryExpression<LEFT_CHILD_TYPE, RIGHT_CHILD_TYPE> {
+
+ enum OperatorPosition {
+ BINARY_INFIX,
+ UNARY_PREFIX,
+ UNARY_POSTFIX,
+ }
+
+ /**
+ * All counts as expressions.
+ */
+ @SuppressWarnings("checkstyle:RegexpSingleline")
+ public enum Operator {
+ MULTIPLY("*", "multiply", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.MULTIPLY),
+ DIVIDE("/", "divide", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.DIVIDE),
+ MOD("%", "mod", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.MOD),
+ INT_DIVIDE("DIV", "int_divide", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.INT_DIVIDE),
+ ADD("+", "add", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.ADD),
+ SUBTRACT("-", "subtract", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.SUBTRACT),
+ BITAND("&", "bitand", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.BITAND),
+ BITOR("|", "bitor", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.BITOR),
+ BITXOR("^", "bitxor", Arithmetic.OperatorPosition.BINARY_INFIX, TExprOpcode.BITXOR),
+ BITNOT("~", "bitnot", Arithmetic.OperatorPosition.UNARY_PREFIX, TExprOpcode.BITNOT),
+ FACTORIAL("!", "factorial", Arithmetic.OperatorPosition.UNARY_POSTFIX, TExprOpcode.FACTORIAL);
Review Comment:
these maybe a UnaryExpression? For the more, based on the previous discussion, we should break them down into separate expressions, should we @EmmyMiao87 ?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -132,11 +153,18 @@ private LogicalPlan plan(ParserRuleContext tree) {
public LogicalPlan visitQuery(QueryContext ctx) {
Supplier<LogicalPlan> f = () -> {
// TODO: need to add withQueryResultClauses and withCTE
- return plan(ctx.queryTerm());
+ LogicalPlan query = plan(ctx.queryTerm());
+ LogicalPlan queryOrganization = withQueryOrganization(ctx.queryOrganization(), query);
+ return queryOrganization == null ? query : queryOrganization;
Review Comment:
should we move this statement into withQueryOrganization
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -132,11 +153,18 @@ private LogicalPlan plan(ParserRuleContext tree) {
public LogicalPlan visitQuery(QueryContext ctx) {
Supplier<LogicalPlan> f = () -> {
// TODO: need to add withQueryResultClauses and withCTE
- return plan(ctx.queryTerm());
+ LogicalPlan query = plan(ctx.queryTerm());
+ LogicalPlan queryOrganization = withQueryOrganization(ctx.queryOrganization(), query);
+ return queryOrganization == null ? query : queryOrganization;
};
return ParserUtils.withOrigin(ctx, f);
}
+ private LogicalPlan withQueryOrganization(QueryOrganizationContext ctx, LogicalPlan children) {
+ List<SortItems> sortItems = visitQueryOrganization(ctx);
+ return sortItems == null ? null : new LogicalUnaryPlan(new LogicalSort(sortItems), children);
Review Comment:
maybe it is better
```suggestion
return sortItems == null ? children : new LogicalUnaryPlan(new LogicalSort(sortItems), children);
```
--
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