[GitHub] [pinot] xiangfu0 commented on a diff in pull request #11036: [multistage] clean up aggregate v1 function for v2 planner

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

xiangfu0 commented on code in PR #11036:
URL: https://github.com/apache/pinot/pull/11036#discussion_r1254804032


##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -29,12 +29,28 @@ public class PinotHintOptions {
   public static final String AGGREGATE_HINT_OPTIONS = "aggOptions";
   public static final String JOIN_HINT_OPTIONS = "joinOptions";
 
+  /**
+   * Hint to denote that the aggregation node is the final aggregation stage which extracts the final result.
+   */
+  public static final String INTERNAL_AGG_OPTIONS = "aggOptionsInternal";
+
   private PinotHintOptions() {
     // do not instantiate.
   }
 
+  public static class InternalAggregateOptions {
+    public static final String AGG_TYPE = "agg_type";
+    public enum AggType {
+      DIRECT,

Review Comment:
   Can you add more comments on the type to distinguish `DIRECT` and `LEAF`?



##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/plannode/AggregateNode.java:
##########
@@ -115,29 +72,13 @@ public List<RelHint> getRelHints() {
     return _relHints;
   }
 
-  public AggregationStage getAggregationStage() {
-    return _aggregationStage;
-  }
-
-  public boolean isTreatIntermediateStageAsLeaf() {
-    return _treatIntermediateStageAsLeaf;
-  }
-
-  public boolean isFinalStage() {
-    return _aggregationStage == AggregationStage.FINAL;
-  }
-
-  public boolean isIntermediateStage() {
-    return _aggregationStage == AggregationStage.INTERMEDIATE;
-  }
-
-  public boolean isLeafStage() {
-    return _aggregationStage == AggregationStage.LEAF;
+  public PinotHintOptions.InternalAggregateOptions.AggType getAggType() {
+    return _aggType;
   }
 
   @Override
   public String explain() {
-    return "AGGREGATE";
+    return "AGGREGATE" + _aggType;

Review Comment:
   `"AGGREGATE_" + _aggType` ?



##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/hint/PinotHintOptions.java:
##########
@@ -29,12 +29,28 @@ public class PinotHintOptions {
   public static final String AGGREGATE_HINT_OPTIONS = "aggOptions";
   public static final String JOIN_HINT_OPTIONS = "joinOptions";
 
+  /**
+   * Hint to denote that the aggregation node is the final aggregation stage which extracts the final result.
+   */
+  public static final String INTERNAL_AGG_OPTIONS = "aggOptionsInternal";
+
   private PinotHintOptions() {
     // do not instantiate.
   }
 
+  public static class InternalAggregateOptions {
+    public static final String AGG_TYPE = "agg_type";
+    public enum AggType {

Review Comment:
   suggest add boolean methods like: isFinal(), isLeaf()... for comparison simplicity. 



##########
pinot-query-planner/src/main/java/org/apache/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java:
##########
@@ -248,12 +227,12 @@ private RelNode makeNewIntermediateAgg(RelOptRuleCall ruleCall, Aggregate oldAgg
    */
   private static void convertIntermediateAggCall(RexBuilder rexBuilder, Aggregate oldAggRel, int oldCallIndex,
       AggregateCall oldCall, List<AggregateCall> newCalls, Map<AggregateCall, RexNode> aggCallMapping,
-      boolean isLeafStageAggregationPresent, List<Integer> argList, PinotLogicalExchange exchange) {
+      AggType aggType, List<Integer> argList, PinotLogicalExchange exchange) {
     final int nGroups = oldAggRel.getGroupCount();
     final SqlAggFunction oldAggregation = oldCall.getAggregation();
 
     List<Integer> newArgList;
-    if (isLeafStageAggregationPresent) {
+    if (AggType.INTERMEDIATE.equals(aggType) || AggType.FINAL.equals(aggType)) {

Review Comment:
   worth a static method `isLeafStageAggregation()`  for 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@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org