Copilot commented on code in PR #16643:
URL: https://github.com/apache/iotdb/pull/16643#discussion_r2467724201
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/TVList.java:
##########
@@ -808,10 +962,19 @@ public boolean hasNextBatch() {
@Override
public TsBlock nextBatch() {
TSDataType dataType = getDataType();
- TsBlockBuilder builder = new
TsBlockBuilder(Collections.singletonList(dataType));
+ int maxRowCountOfCurrentBatch =
+ Math.min(
+ paginationController.hasLimit()
+ ? (int) paginationController.getCurLimit()
+ : Integer.MAX_VALUE,
+ Math.min(maxNumberOfPointsInPage, rows - index));
Review Comment:
[nitpick] The nested Math.min calls make this calculation hard to read.
Consider using Math.min with three arguments or extracting intermediate
variables to improve clarity.
```suggestion
int limit = paginationController.hasLimit() ? (int)
paginationController.getCurLimit() : Integer.MAX_VALUE;
int remainingRows = rows - index;
int maxRowCountOfCurrentBatch = Math.min(limit,
Math.min(maxNumberOfPointsInPage, remainingRows));
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/TVList.java:
##########
@@ -808,10 +962,19 @@ public boolean hasNextBatch() {
@Override
public TsBlock nextBatch() {
TSDataType dataType = getDataType();
- TsBlockBuilder builder = new
TsBlockBuilder(Collections.singletonList(dataType));
+ int maxRowCountOfCurrentBatch =
+ Math.min(
+ paginationController.hasLimit()
+ ? (int) paginationController.getCurLimit()
+ : Integer.MAX_VALUE,
+ Math.min(maxNumberOfPointsInPage, rows - index));
+ TsBlockBuilder builder =
+ new TsBlockBuilder(maxRowCountOfCurrentBatch,
Collections.singletonList(dataType));
switch (dataType) {
case BOOLEAN:
- while (index < rows && builder.getPositionCount() <
maxNumberOfPointsInPage) {
+ while (index < rows
+ && builder.getPositionCount() < maxNumberOfPointsInPage
Review Comment:
The condition `builder.getPositionCount() < maxNumberOfPointsInPage` is now
redundant since maxRowCountOfCurrentBatch already accounts for both
maxNumberOfPointsInPage and the pagination limit. This check is performed on
every iteration unnecessarily.
```suggestion
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/AlignedTVList.java:
##########
@@ -1632,33 +1635,41 @@ public TsBlock nextBatch() {
if (validRowCount >= maxNumberOfPointsInPage ||
isCurrentTimeExceedTimeRange(time)) {
break;
}
- // skip empty row
- if (allValueColDeletedMap != null
- &&
allValueColDeletedMap.isMarked(getValueIndex(getScanOrderIndex(index)))) {
- continue;
- }
- if (!isTimeSatisfied(time)) {
+ // skip invalid row
+ if ((allValueColDeletedMap != null
+ &&
allValueColDeletedMap.isMarked(getValueIndex(getScanOrderIndex(index))))
+ || !isTimeSatisfied(time)) {
+ timeInvalidInfo =
+ timeInvalidInfo == null
+ ? new LazyBitMap(index, maxRowCountOfCurrentBatch, rows - 1)
+ : timeInvalidInfo;
+ timeInvalidInfo.mark(index);
continue;
}
int nextRowIndex = index + 1;
while (nextRowIndex < rows
&& ((allValueColDeletedMap != null
&& allValueColDeletedMap.isMarked(
getValueIndex(getScanOrderIndex(nextRowIndex))))
- || !isTimeSatisfied(time))) {
+ ||
!isTimeSatisfied(getTime(getScanOrderIndex(nextRowIndex))))) {
Review Comment:
The condition evaluates
`isTimeSatisfied(getTime(getScanOrderIndex(nextRowIndex)))` but the loop header
already checked that `nextRowIndex < rows`. If the time at `nextRowIndex`
doesn't satisfy the filter but the row is not deleted, this will incorrectly
mark it as invalid instead of stopping the duplicate-check loop. The original
code checked `isTimeSatisfied(time)` (the current time), not the next row's
time.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/source/SeriesScanUtil.java:
##########
@@ -193,6 +197,20 @@ public void initQueryDataSource(QueryDataSource
dataSource) {
// init file index
orderUtils.setCurSeqFileIndex(dataSource);
curUnseqFileIndex = 0;
+
+ if (satisfiedTimeRange == null) {
+ long startTime = Long.MAX_VALUE;
+ long endTime = Long.MIN_VALUE;
+ if (scanOptions.getGlobalTimeFilter() == null) {
+ satisfiedTimeRange = new TimeRange(Long.MIN_VALUE, Long.MAX_VALUE);
+ return;
+ }
+ for (TimeRange timeRange : context.getGlobalTimeFilterTimeRanges()) {
+ startTime = Math.min(startTime, timeRange.getMin());
+ endTime = Math.max(endTime, timeRange.getMax());
+ }
+ satisfiedTimeRange = new TimeRange(startTime, endTime);
+ }
Review Comment:
[nitpick] The initialization of `startTime` and `endTime` is unnecessary
when `scanOptions.getGlobalTimeFilter() == null` since the method returns
early. Consider moving these variable declarations inside the else block or
after the null check.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
