You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/08/04 02:34:00 UTC
[GitHub] [iotdb] liuminghui233 commented on a diff in pull request #6840: [IOTDB-3695]Implememtation of having clause
liuminghui233 commented on code in PR #6840:
URL: https://github.com/apache/iotdb/pull/6840#discussion_r937283088
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/expression/multi/FunctionExpression.java:
##########
@@ -512,6 +512,7 @@ private UDFQueryTransformer constructUdfTransformer(
@Override
public boolean isMappable(TypeProvider typeProvider) {
+ if (isBuiltInAggregationFunctionExpression) return true;
Review Comment:
It is better to add a pair of parentheses, even if there is only one statement inside the if
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/planner/LogicalPlanVisitor.java:
##########
@@ -232,12 +237,34 @@ public PlanNode visitQueryBody(
}
}
- planBuilder =
- planBuilder.planTransform(
- transformExpressions,
- queryStatement.isGroupByTime(),
- queryStatement.getSelectComponent().getZoneId(),
- queryStatement.getResultTimeOrder());
+ if (queryStatement.isGroupByLevel()) {
+ if (havingExpression != null) {
Review Comment:
We don't need this judgment here because this judgment will be made in planFilterAndTransform.
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/planner/LogicalPlanVisitor.java:
##########
@@ -284,23 +311,68 @@ public PlanNode visitQueryBody(
measurementIndexes,
analysis.getGroupByLevelExpressions(),
analysis.getTypeProvider());
+ if (queryStatement.isGroupByLevel()) {
+ if (havingExpression != null) {
+ planBuilder = // plan Having with GroupByLevel
+ planBuilder.planFilterAndTransform(
+ havingExpression,
+ analysis.getGroupByLevelExpressions().keySet(),
+ queryStatement.isGroupByTime(),
+ queryStatement.getSelectComponent().getZoneId(),
+ queryStatement.getResultTimeOrder());
+ }
+ } else {
+ if (havingExpression != null) {
+ planBuilder = // plan Having without GroupByLevel
+ planBuilder.planFilterAndTransform(
+ havingExpression,
+ transformExpressions,
+ queryStatement.isGroupByTime(),
+ queryStatement.getSelectComponent().getZoneId(),
+ queryStatement.getResultTimeOrder());
+ }
+ }
} else {
planBuilder =
- planBuilder
- .planAggregationSource(
- sourceExpressions,
- curStep,
- queryStatement.getResultTimeOrder(),
- analysis.getGlobalTimeFilter(),
- analysis.getGroupByTimeParameter(),
- aggregationExpressions,
- analysis.getGroupByLevelExpressions(),
- analysis.getTypeProvider())
- .planTransform(
+ planBuilder.planAggregationSource(
+ sourceExpressions,
+ curStep,
+ queryStatement.getResultTimeOrder(),
+ analysis.getGlobalTimeFilter(),
+ analysis.getGroupByTimeParameter(),
+ aggregationExpressions,
+ analysis.getGroupByLevelExpressions(),
+ analysis.getTypeProvider());
+
+ // TODO condsider to extracted as a planBuild method
+ if (queryStatement.isGroupByLevel()) {
+ if (havingExpression != null) {
Review Comment:
same as above
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/planner/LogicalPlanVisitor.java:
##########
@@ -284,23 +311,68 @@ public PlanNode visitQueryBody(
measurementIndexes,
analysis.getGroupByLevelExpressions(),
analysis.getTypeProvider());
+ if (queryStatement.isGroupByLevel()) {
+ if (havingExpression != null) {
+ planBuilder = // plan Having with GroupByLevel
+ planBuilder.planFilterAndTransform(
+ havingExpression,
+ analysis.getGroupByLevelExpressions().keySet(),
+ queryStatement.isGroupByTime(),
+ queryStatement.getSelectComponent().getZoneId(),
+ queryStatement.getResultTimeOrder());
+ }
+ } else {
+ if (havingExpression != null) {
Review Comment:
same as above
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/analyze/ExpressionAnalyzer.java:
##########
@@ -58,13 +59,50 @@
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.getPairFromBetweenTimeFirst;
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.getPairFromBetweenTimeSecond;
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.getPairFromBetweenTimeThird;
+import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructBinaryExpression;
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructBinaryExpressions;
+import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructFunctionExpression;
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructFunctionExpressions;
+import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructTernaryExpression;
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructTernaryExpressions;
+import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructTimeSeriesOperand;
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructTimeSeriesOperands;
+import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructUnaryExpression;
import static org.apache.iotdb.db.mpp.plan.analyze.ExpressionUtils.reconstructUnaryExpressions;
public class ExpressionAnalyzer {
+ /**
+ * Check if all paths in expression has only one node. If not, throw a {@link SemanticException}.
+ *
+ * @param expression expression to be checked
+ */
+ public static void checkIsAllPathOneNode(Expression expression) {
Review Comment:
This method seems very similar to checkIsAllMeasurement. We can use the same method, let it return a boolean value, and throw a semantic exception externally.
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/planner/LogicalPlanVisitor.java:
##########
@@ -284,23 +311,68 @@ public PlanNode visitQueryBody(
measurementIndexes,
analysis.getGroupByLevelExpressions(),
analysis.getTypeProvider());
+ if (queryStatement.isGroupByLevel()) {
+ if (havingExpression != null) {
Review Comment:
same as above
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/analyze/ExpressionAnalyzer.java:
##########
@@ -498,6 +536,158 @@ public static List<Expression> removeWildcardInQueryFilter(
}
}
+ public static Expression replaceRawPathWithGroupedPath(
+ Expression predicate, Map<Expression, Expression> rawPathToGroupedPathMapInHaving) {
+ if (predicate instanceof TernaryExpression) {
+ Expression firstExpression =
+ replaceRawPathWithGroupedPath(
+ ((TernaryExpression) predicate).getFirstExpression(),
+ rawPathToGroupedPathMapInHaving);
+ Expression secondExpression =
+ replaceRawPathWithGroupedPath(
+ ((TernaryExpression) predicate).getSecondExpression(),
+ rawPathToGroupedPathMapInHaving);
+ Expression thirdExpression =
+ replaceRawPathWithGroupedPath(
+ ((TernaryExpression) predicate).getThirdExpression(),
+ rawPathToGroupedPathMapInHaving);
+ return reconstructTernaryExpression(
+ predicate, firstExpression, secondExpression, thirdExpression);
+ } else if (predicate instanceof BinaryExpression) {
+ Expression leftExpression =
+ replaceRawPathWithGroupedPath(
+ ((BinaryExpression) predicate).getLeftExpression(), rawPathToGroupedPathMapInHaving);
+ Expression rightExpression =
+ replaceRawPathWithGroupedPath(
+ ((BinaryExpression) predicate).getRightExpression(), rawPathToGroupedPathMapInHaving);
+ return reconstructBinaryExpression(
+ predicate.getExpressionType(), leftExpression, rightExpression);
+ } else if (predicate instanceof UnaryExpression) {
+ Expression expression =
+ replaceRawPathWithGroupedPath(
+ ((UnaryExpression) predicate).getExpression(), rawPathToGroupedPathMapInHaving);
+ return reconstructUnaryExpression((UnaryExpression) predicate, expression);
+ } else if (predicate instanceof FunctionExpression) {
+ List<Expression> expressions = predicate.getExpressions();
+ List<Expression> childrenExpressions = new ArrayList<>();
+ for (Expression expression : expressions) {
+ childrenExpressions.add(
+ replaceRawPathWithGroupedPath(expression, rawPathToGroupedPathMapInHaving));
+ }
+ return reconstructFunctionExpression((FunctionExpression) predicate, childrenExpressions);
+ } else if (predicate instanceof TimeSeriesOperand) {
+ PartialPath groupedPath =
+ ((TimeSeriesOperand) rawPathToGroupedPathMapInHaving.get(predicate)).getPath();
+ return reconstructTimeSeriesOperand(groupedPath);
+ } else if (predicate instanceof TimestampOperand || predicate instanceof ConstantOperand) {
+ return predicate;
+ } else {
+ throw new IllegalArgumentException(
+ "unsupported expression type: " + predicate.getExpressionType());
+ }
+ }
+
+ /**
+ * Concat suffix path in Having clause with the prefix path in the FROM clause. And then, bind
+ * schema ({@link PartialPath} -> {@link MeasurementPath}) and removes wildcards in Expression.
+ *
+ * @param prefixPaths prefix paths in the FROM clause
+ * @param schemaTree interface for querying schema information
+ * @param typeProvider a map to record output symbols and their data types
+ * @return the expression list with full path and after binding schema
+ */
+ public static List<Expression> removeWildcardInHavingExpression(
Review Comment:
This method is very similar to `removeWildcardInQueryFilter`, and we need to consider using one. For example, we should do some semantic checks upfront.
##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/analyze/ExpressionAnalyzer.java:
##########
@@ -681,6 +871,98 @@ public static List<Expression> removeWildcardInQueryFilterByDevice(
}
}
+ /**
+ * Concat measurement in Having clause with device path. And then, bind schema ({@link
+ * PartialPath} -> {@link MeasurementPath}) and removes wildcards.
+ *
+ * @return the expression list with full path and after binding schema
+ */
+ public static List<Expression> removeWildcardInHavingExpressionByDevice(
Review Comment:
This method is very similar to `removeWildcardInQueryFilterByDevice`, and we need to consider using one. For example, we should do some semantic checks upfront.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org