[GitHub] [iotdb] liuminghui233 commented on a diff in pull request #8975: [IOTDB-5456]Implement COUNT_IF built-in aggregation function

["liuminghui233 (via GitHub)" <gi...@apache.org>]

liuminghui233 commented on code in PR #8975:
URL: https://github.com/apache/iotdb/pull/8975#discussion_r1098348865


##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/analyze/ExpressionAnalyzer.java:
##########
@@ -1066,16 +1105,27 @@ public static Expression removeAliasFromExpression(Expression expression) {
     }
   }
 
-  /** Check for arithmetic expression, logical expression, UDF. Returns true if it exists. */
-  public static boolean checkIsNeedTransform(Expression expression) {
+  /**
+   * Check for arithmetic expression, logical expression, UDF. Returns true if it exists.
+   *
+   * @param isAfterAggregation if transform is after Aggregation node, all AggregationFunctions are
+   *     not need to do
+   */
+  public static boolean checkIsNeedTransform(Expression expression, boolean isAfterAggregation) {

Review Comment:
   It's better to add a new method instead of adding logic to this method



##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/analyze/ExpressionAnalyzer.java:
##########
@@ -511,7 +532,7 @@ public static List<Expression> removeWildcardInFilter(
       List<Expression> rightExpressions =
           removeWildcardInFilter(
               ((BinaryExpression) predicate).getRightExpression(), prefixPaths, schemaTree, false);
-      if (isRoot && predicate.getExpressionType() == ExpressionType.LOGIC_AND) {
+      if ((isRoot && predicate.getExpressionType() == ExpressionType.LOGIC_AND)) {

Review Comment:
   redundant parentheses



##########
node-commons/src/main/java/org/apache/iotdb/commons/udf/builtin/BuiltinFunction.java:
##########
@@ -56,4 +56,14 @@ public String getFunctionName() {
   public static Set<String> getNativeFunctionNames() {
     return NATIVE_FUNCTION_NAMES;
   }
+
+  public static boolean isMappable(String name) {

Review Comment:
   add a comment to this method



##########
node-commons/src/main/java/org/apache/iotdb/commons/udf/builtin/BuiltinAggregationFunction.java:
##########
@@ -56,4 +57,48 @@ public String getFunctionName() {
   public static Set<String> getNativeFunctionNames() {
     return NATIVE_FUNCTION_NAMES;
   }
+
+  /** @return if the Aggregation can use statistics to optimize */
+  public static boolean canUseStatistics(String name) {
+    final String functionName = name.toLowerCase();
+    switch (functionName) {
+      case "min_time":
+      case "max_time":
+      case "max_value":
+      case "min_value":
+      case "extreme":
+      case "first_value":
+      case "last_value":
+      case "count":
+      case "avg":
+      case "sum":
+        return true;
+      case "count_if":
+        return false;
+      default:
+        throw new IllegalArgumentException("Invalid Aggregation function: " + name);
+    }
+  }
+
+  /** @return if the Aggregation can split to multi phases */
+  public static boolean canSplitToMultiPhases(String name) {

Review Comment:
   ```suggestion
     public static boolean canMergePartialResult(String name) {
   ```
   Whether the two method names are equivalent? @JackieTien97 @Wei-hao-Li 



##########
server/src/main/java/org/apache/iotdb/db/mpp/plan/planner/LogicalPlanVisitor.java:
##########
@@ -215,7 +215,7 @@ public PlanNode visitQueryBody(
       // aggregation query
       boolean isRawDataSource =
           analysis.hasValueFilter()
-              || needTransform(sourceTransformExpressions)
+              || needTransform(sourceTransformExpressions, false)

Review Comment:
   add a new judgment



-- 
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