You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2020/10/19 06:13:47 UTC
[GitHub] [iotdb] samperson1997 commented on a change in pull request #1824: [IOTDB-942] Optimization of query with long time unsequence page
samperson1997 commented on a change in pull request #1824:
URL: https://github.com/apache/iotdb/pull/1824#discussion_r507483827
##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -931,11 +1043,31 @@ public TsFileResource getNextSeqFileResource(List<TsFileResource> seqResources,
(c1, c2) -> Long.compare(keyExtractor.applyAsLong(c1), keyExtractor.applyAsLong(c2));
}
+ @Override
+ public long getCurrentEndPoint(long time, Statistics<? extends Object> statistics) {
+ return Math.min(time, statistics.getEndTime());
+ }
+
+ @Override
+ public long getCurrentEndPoint(Statistics<? extends Object> seqStatistics,
+ Statistics<? extends Object> unseqStatistics) {
+ return Math.min(seqStatistics.getEndTime(), unseqStatistics.getEndTime());
+ }
+
@Override
public boolean isExcessEndpoint(long time, long endpointTime) {
return time > endpointTime;
}
+ @Override
+ public boolean isTakeSeqAsFirst(Statistics<? extends Object> seqStatistics,
+ Statistics<? extends Object> unseqStatistics) {
+ if (seqStatistics.getStartTime() < unseqStatistics.getStartTime()) {
+ return true;
+ }
+ return false;
Review comment:
```suggestion
return seqStatistics.getStartTime() < unseqStatistics.getStartTime());
```
##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -870,12 +963,31 @@ public TsFileResource getNextSeqFileResource(List<TsFileResource> seqResources,
(c1, c2) -> Long.compare(keyExtractor.applyAsLong(c2), keyExtractor.applyAsLong(c1));
}
+ @Override
+ public long getCurrentEndPoint(long time, Statistics<? extends Object> statistics) {
+ return Math.max(time, statistics.getStartTime());
+ }
+
+ @Override
+ public long getCurrentEndPoint(Statistics<? extends Object> seqStatistics,
+ Statistics<? extends Object> unseqStatistics) {
+ return Math.max(seqStatistics.getStartTime(), unseqStatistics.getStartTime());
+ }
@Override
public boolean isExcessEndpoint(long time, long endpointTime) {
return time < endpointTime;
}
+ @Override
+ public boolean isTakeSeqAsFirst(Statistics<? extends Object> seqStatistics,
+ Statistics<? extends Object> unseqStatistics) {
+ if (seqStatistics.getEndTime() > unseqStatistics.getEndTime()) {
+ return true;
+ }
+ return false;
Review comment:
```suggestion
return seqStatistics.getEndTime() > unseqStatistics.getEndTime();
```
##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -816,8 +901,16 @@ boolean isModified() {
<T> Comparator<T> comparingLong(ToLongFunction<? super T> keyExtractor);
+ long getCurrentEndPoint(long time, Statistics<? extends Object> statistics);
+
+ long getCurrentEndPoint(Statistics<? extends Object> seqStatistics,
+ Statistics<? extends Object> unseqStatistics);
+
boolean isExcessEndpoint(long time, long endpointTime);
+ boolean isTakeSeqAsFirst(Statistics<? extends Object> seqStatistics,
Review comment:
Add java doc for these new abstract methods
##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -407,11 +409,15 @@ boolean hasNextPage() throws IOException {
}
// make sure firstPageReader won't be null while cachedPageReaders has more cached page readers
- while (firstPageReader == null && !cachedPageReaders.isEmpty()) {
- firstPageReader = cachedPageReaders.poll();
- if (!cachedPageReaders.isEmpty()
- && orderUtils.isOverlapped(firstPageReader.getStatistics(),
- cachedPageReaders.peek().getStatistics())) {
+ while (firstPageReader == null && (!seqPageReaders.isEmpty() || !unSeqPageReaders.isEmpty())) {
+
+ initFirstPageReader();
+
+ if ((!seqPageReaders.isEmpty() && orderUtils
+ .isOverlapped(firstPageReader.getStatistics(), seqPageReaders.get(0).getStatistics()))
+ || (!unSeqPageReaders.isEmpty() && orderUtils
+ .isOverlapped(firstPageReader.getStatistics(),
+ unSeqPageReaders.peek().getStatistics()))) {
Review comment:
better to extract these complicate condition statements into a private method, since they appear not only once in this file (for example, in lines 395-398)
##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -610,18 +674,33 @@ private void tryToPutAllDirectlyOverlappedPageReadersIntoMergeReader() throws IO
}
/*
- * put all currently directly overlapped page reader to merge reader
+ * put all currently directly overlapped unseq page reader to merge reader
*/
- unpackAllOverlappedCachedPageReadersToMergeReader(currentPageEndpointTime);
+ unpackAllOverlappedUnseqPageReadersToMergeReader(currentPageEndpointTime);
}
- private void unpackAllOverlappedCachedPageReadersToMergeReader(long endpointTime)
+ private void initFirstPageReader() {
+ if (!seqPageReaders.isEmpty() && !unSeqPageReaders.isEmpty()) {
+ if (orderUtils.isTakeSeqAsFirst(seqPageReaders.get(0).getStatistics(), unSeqPageReaders.peek()
+ .getStatistics())) {
+ firstPageReader = seqPageReaders.remove(0);
+ } else {
+ firstPageReader = unSeqPageReaders.poll();
+ }
+ } else if (!seqPageReaders.isEmpty()) {
+ firstPageReader = seqPageReaders.remove(0);
+ } else if (!unSeqPageReaders.isEmpty()) {
+ firstPageReader = unSeqPageReaders.poll();
+ }
+ }
+
+ private void unpackAllOverlappedUnseqPageReadersToMergeReader(long endpointTime)
throws IOException {
- while (!cachedPageReaders.isEmpty()
- && orderUtils.isOverlapped(endpointTime, cachedPageReaders.peek().data.getStatistics())) {
- putPageReaderToMergeReader(cachedPageReaders.poll());
+ while (!unSeqPageReaders.isEmpty()
+ && orderUtils.isOverlapped(endpointTime, unSeqPageReaders.peek().data.getStatistics())) {
+ putPageReaderToMergeReader(unSeqPageReaders.poll());
}
- if (firstPageReader != null &&
+ if (firstPageReader != null && !firstPageReader.isSeq &&
Review comment:
```suggestion
if (firstPageReader != null && !firstPageReader.isSeq() &&
```
##########
File path: server/src/main/java/org/apache/iotdb/db/query/reader/series/SeriesReader.java
##########
@@ -381,19 +383,19 @@ boolean hasNextPage() throws IOException {
/*
* first chunk metadata is already unpacked, consume cached pages
*/
- if (!cachedPageReaders.isEmpty()) {
- firstPageReader = cachedPageReaders.poll();
+ initFirstPageReader();
+ if (firstPageReader != null) {
long endpointTime = orderUtils.getOverlapCheckTime(firstPageReader.getStatistics());
unpackAllOverlappedTsFilesToTimeSeriesMetadata(endpointTime);
unpackAllOverlappedTimeSeriesMetadataToCachedChunkMetadata(endpointTime, false);
unpackAllOverlappedChunkMetadataToCachedPageReaders(endpointTime, false);
Review comment:
Since you modified the field name of `cachedPageReaders`, maybe the name of this method should be updated as well.
----------------------------------------------------------------
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