JackieTien97 commented on a change in pull request #1650: URL: https://github.com/apache/incubator-iotdb/pull/1650#discussion_r486749377
########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; } } else { + cachedTimestamps.addFirst(timestamp); Review comment: It seems that this timestamp is useless, we don't need to put it back to `cachedTimestamps`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; Review comment: It seems there will be IndexOutOfArrayException here. What if `cachedTimestamps`'s size is larger than `timeStampFetchSize`. And also, you call the `constructTimeArrayForOneCal` method in the following and fetch another `timeStampFetchSize` timestamp, i think it should be changed in case of `desc`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; } } else { + cachedTimestamps.addFirst(timestamp); Review comment: It seems that this timestamp is useless, we don't need to put it back to `cachedTimestamps`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; Review comment: It seems there will be IndexOutOfArrayException here. What if `cachedTimestamps`'s size is larger than `timeStampFetchSize`. And also, you call the `constructTimeArrayForOneCal` method in the following and fetch another `timeStampFetchSize` timestamp, i think it should be changed in case of `desc`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; } } else { + cachedTimestamps.addFirst(timestamp); Review comment: It seems that this timestamp is useless, we don't need to put it back to `cachedTimestamps`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; Review comment: It seems there will be IndexOutOfArrayException here. What if `cachedTimestamps`'s size is larger than `timeStampFetchSize`. And also, you call the `constructTimeArrayForOneCal` method in the following and fetch another `timeStampFetchSize` timestamp, i think it should be changed in case of `desc`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; } } else { + cachedTimestamps.addFirst(timestamp); Review comment: It seems that this timestamp is useless, we don't need to put it back to `cachedTimestamps`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; Review comment: It seems there will be IndexOutOfArrayException here. What if `cachedTimestamps`'s size is larger than `timeStampFetchSize`. And also, you call the `constructTimeArrayForOneCal` method in the following and fetch another `timeStampFetchSize` timestamp, i think it should be changed in case of `desc`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; } } else { + cachedTimestamps.addFirst(timestamp); Review comment: It seems that this timestamp is useless, we don't need to put it back to `cachedTimestamps`. ########## File path: server/src/main/java/org/apache/iotdb/db/query/dataset/groupby/GroupByWithValueFilterDataSet.java ########## @@ -122,13 +127,18 @@ protected RowRecord nextWithoutConstraint() throws IOException { long[] timestampArray = new long[timeStampFetchSize]; int timeArrayLength = 0; - if (hasCachedTimestamp) { + while (!cachedTimestamps.isEmpty()) { + long timestamp = cachedTimestamps.remove(); if (timestamp < curEndTime) { + if (!groupByTimePlan.isAscending() && timestamp < curStartTime) { + cachedTimestamps.addFirst(timestamp); + return constructRowRecord(aggregateResultList); + } if (timestamp >= curStartTime) { - hasCachedTimestamp = false; timestampArray[timeArrayLength++] = timestamp; Review comment: It seems there will be IndexOutOfArrayException here. What if `cachedTimestamps`'s size is larger than `timeStampFetchSize`. And also, you call the `constructTimeArrayForOneCal` method in the following and fetch another `timeStampFetchSize` timestamp, i think it should be changed in case of `desc`. ---------------------------------------------------------------- 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: us...@infra.apache.org