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/19 07:33:05 UTC
[GitHub] [doris] morrySnow commented on a diff in pull request #10981: [Enhancement](Nereids) add some basic data structure definitions.
morrySnow commented on code in PR #10981:
URL: https://github.com/apache/doris/pull/10981#discussion_r924041187
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -353,6 +384,31 @@ public UnboundFunction visitFunctionCall(DorisParser.FunctionCallContext ctx) {
});
}
+ @Override
+ public Expression visitInterval(IntervalContext ctx) {
+ return new IntervalLiteral((Expression) visit(ctx.value), (String) visit(ctx.unit));
Review Comment:
```suggestion
return new IntervalLiteral(getExpression(ctx.value), visitUnitIdentifier(ctx.unit));
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -353,6 +384,31 @@ public UnboundFunction visitFunctionCall(DorisParser.FunctionCallContext ctx) {
});
}
+ @Override
+ public Expression visitInterval(IntervalContext ctx) {
+ return new IntervalLiteral((Expression) visit(ctx.value), (String) visit(ctx.unit));
+ }
+
+ @Override
+ public String visitUnitIdentifier(UnitIdentifierContext ctx) {
+ return ctx.getText();
+ }
+
+ @Override
+ public Expression visitTypeConstructor(TypeConstructorContext ctx) {
+ String value = ctx.STRING().getText();
+ value = value.substring(1, value.length() - 1);
+ String type = ctx.identifier().getText().toUpperCase();
+ switch (type) {
+ case "DATE":
+ return new DateLiteral(value, DateType.INSTANCE);
+ case "DATETIME":
+ return new DateLiteral(value, DateTimeType.INSTANCE);
Review Comment:
why use one class to represent two type?
##########
fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -220,6 +222,13 @@ booleanValue
: TRUE | FALSE
Review Comment:
this line is duplicated with `booleanValue`, please help to remove it, thx
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/IntervalLiteral.java:
##########
@@ -0,0 +1,42 @@
+// 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;
+
+/**
+ * Interval for timestamp calculation.
+ */
+public class IntervalLiteral extends Expression {
+ private final Expression value;
+ private final String timeUnit;
Review Comment:
do we need add a unit enum?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/Expression.java:
##########
@@ -83,6 +84,14 @@ public boolean isConstant() {
return children().stream().anyMatch(Expression::isConstant);
}
+ public final Expression castTo(DataType targetType) throws AnalysisException {
+ return uncheckedCastTo(targetType);
+ }
+
+ protected Expression uncheckedCastTo(DataType targetType) throws AnalysisException {
+ throw new RuntimeException();
Review Comment:
```suggestion
throw new RuntimeException("Do not implement uncheckedCastTo");
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindFunction.java:
##########
@@ -93,5 +104,25 @@ public Expression visitUnboundFunction(UnboundFunction unboundFunction, Void con
}
return unboundFunction;
}
+
+ @Override
+ public Expression visitTimestampArithmetic(TimestampArithmetic arithmetic, Void context) {
+ String funcOpName = null;
+ if (arithmetic.getFuncName() == null) {
+ funcOpName = String.format("%sS_%s", arithmetic.getTimeUnit(),
+ (arithmetic.getOp() == Operator.ADD) ? "ADD" : "SUB");
+ }
Review Comment:
do we need else
```java
else {
funcOpName = arithmetic.getFuncName();
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DateType.java:
##########
@@ -0,0 +1,39 @@
+// 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.types;
+
+import org.apache.doris.catalog.Type;
+
+/**
+ * Date type in Nereids.
+ */
+public class DateType extends PrimitiveType {
+
+ public static DateType INSTANCE = new DateType();
+
+ @Override
+ public Type toCatalogDataType() {
+ return Type.DATE;
+ }
+
+ @Override
+ public boolean equals(Object o) {
Review Comment:
why need to override equals
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/DataType.java:
##########
@@ -52,6 +52,12 @@ public static DataType convertFromCatalogDataType(Type catalogType) {
return VarcharType.createVarcharType(scalarType.getLength());
case STRING:
return StringType.INSTANCE;
+ case DATE:
+ return DateType.INSTANCE;
+ case DATETIME:
+ return DateTimeType.INSTANCE;
+ case DECIMALV2:
+ return DecimalType.INSTANCE;
Review Comment:
Decimal need to set precision and scale. BTW, we have DecimalV3 now. i.e. DECIMAL32, DECIMAL64 and DECIMAL128
--
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