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
