[GitHub] [iotdb] JackieTien97 commented on a change in pull request #2029: [IOTDB-825] aggregation by natural month

[GitBox <gi...@apache.org>]

JackieTien97 commented on a change in pull request #2029:
URL: https://github.com/apache/iotdb/pull/2029#discussion_r526733767



##########
File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByEngineDataSet.java
##########
@@ -70,36 +78,72 @@ public GroupByEngineDataSet(QueryContext context, GroupByTimePlan groupByTimePla
       long intervalNum = (long) Math.ceil(queryRange / (double) slidingStep);
       curStartTime = slidingStep * (intervalNum - 1) + startTime;
     }
-    curEndTime = Math.min(curStartTime + interval, endTime);
+
+
+    if (isIntervalByMonth) {
+      //if is group by months interval and sliding step are calculated in ms by * 30 * 86400_000L
+      //now converting them back to integer months
+      interval = interval / msToMonth;
+      //calculate interval length by natural month based on curStartTime
+      //ie. startTIme = 1/31, interval = 1mo, curEndTime will be set to 2/29
+      curEndTime = Math.min(curStartTime + calcIntervalByMonth(interval, curStartTime), endTime);
+    } else {
+      curEndTime = Math.min(curStartTime + interval, endTime);
+    }
+    if (isSlidingStepByMonth) {
+      slidingStep = slidingStep / msToMonth;
+    }
     this.hasCachedTimeInterval = true;
   }
 
   @Override
   protected boolean hasNextWithoutConstraint() {
+    long curSlidingStep = slidingStep;
+    long curInterval = interval;
     // has cached
     if (hasCachedTimeInterval) {
       return true;
     }
 
+    intervalTimes++;
+    //group by natural month, given startTime recalculate interval and sliding step
+    if (isIntervalByMonth) {
+      curInterval = calcIntervalByMonth(intervalTimes * slidingStep + interval, startTime);
+    }
+    if (isSlidingStepByMonth) {
+      curSlidingStep = calcIntervalByMonth(slidingStep * intervalTimes, curStartTime);
+    }
+
     // check if the next interval out of range
     if (ascending) {
-      curStartTime += slidingStep;
+      curStartTime += curSlidingStep;
       //This is an open interval , [0-100)
       if (curStartTime >= endTime) {
         return false;
       }
     } else {
-      curStartTime -= slidingStep;
+      curStartTime -= curSlidingStep;
       if (curStartTime < startTime) {
         return false;
       }
     }
 
     hasCachedTimeInterval = true;
-    curEndTime = Math.min(curStartTime + interval, endTime);
+    if (isIntervalByMonth) {
+      curEndTime = Math.min(startTime + curInterval, endTime);
+    } else {
+      curEndTime = Math.min(curStartTime + curInterval, endTime);
+    }
     return true;
   }
 
+  public long calcIntervalByMonth(long numMonths, long curStartTime) {
+    Calendar calendar = Calendar.getInstance();
+    calendar.setTimeInMillis(startTime);
+    calendar.add(Calendar.MONTH, (int) (numMonths));
+    return calendar.getTimeInMillis() - curStartTime;
+  }
+

Review comment:
       I think you can directly return the `calendar.getTimeInMillis()` instead of the delta to `curStartTime`, because it seems that you still add the curStartTime each time you call the `calcIntervalByMonth` function.
   
   And in this function, it's better to use `calendar.setTimeInMillis(curStartTime);` instead of `startTime`, if so the attribute `intervalTimes` can be deleted.

##########
File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByEngineDataSet.java
##########
@@ -43,6 +45,10 @@
   protected boolean hasCachedTimeInterval;
 
   protected boolean leftCRightO;
+  private boolean isIntervalByMonth = false;
+  private boolean isSlidingStepByMonth = false;
+  protected int intervalTimes;
+  private final long msToMonth = 30 * 86400_000L;

Review comment:
       ```suggestion
     private static final long MS_T0_MONTH = 30 * 86400_000L;
   ```

##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/LogicalGenerator.java
##########
@@ -1411,6 +1418,16 @@ private Long parseDuration(String durationStr) {
           i++;
           unit += durationStr.charAt(i);
         }
+        if (unit.toLowerCase().equals("mo")) {
+          //interval is by month, sliding step by default equals to interval
+          if (!isParsingSlidingStep) {
+            queryOp.setIntervalByMonth(true);
+          }
+          queryOp.setSlidingStepByMonth(true);
+        } else if (isParsingSlidingStep) {
+          //parsing sliding step value, and unit is not by month
+          queryOp.setSlidingStepByMonth(false);
+        }

Review comment:
       I think the logic here should be moved to the enterGroupByTimeClause() function, because only group by time query has the two attributes: `isIntervalByMonth` and `isSlidingStepByMonth` .
   And if so, `isParsingSlidingStep` is useless.




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

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