Copilot commented on code in PR #17304:
URL: https://github.com/apache/iotdb/pull/17304#discussion_r2944868403
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/operator/OperatorContext.java:
##########
@@ -177,7 +176,7 @@ public void recordSpecifiedInfo(String key, String value) {
}
public Map<String, String> getSpecifiedInfo() {
- return specifiedInfo == null ? new HashMap<>() : specifiedInfo;
+ return specifiedInfo == null ? specifiedInfo = new ConcurrentHashMap<>() :
specifiedInfo;
}
Review Comment:
`getSpecifiedInfo()` lazily initializes `specifiedInfo` via assignment
inside a ternary. Since this map is accessed concurrently (as noted in
`recordSpecifiedInfo`), this unsynchronized lazy init can still race and
briefly create/lose maps (and it’s also hard to read). Prefer initializing
`specifiedInfo` eagerly (e.g., final `ConcurrentHashMap`) or use a thread-safe
lazy-init pattern (synchronized/AtomicReference) so all threads share the same
map deterministically.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/TVList.java:
##########
@@ -854,6 +857,10 @@ protected void skipToCurrentTimeRangeStartPosition() {
}
if (searchTimestamp > outer.getMaxTime()) {
// all satisfied data has been consumed
+ // the data with same timestamp could exist multi times, which means
filter multi times
+ if (this.getQueryContext().isVerbose()) {
+
this.getQueryContext().getQueryStatistics().addFilteredRowsOfRowLevel(rows -
index);
+ }
Review Comment:
`queryContext` can be null (e.g., iterators created for flushing/encoding
pass null via MemPointIteratorFactory), but this code calls
`getQueryContext().isVerbose()` unconditionally, which can trigger a
NullPointerException during `hasNextTimeValuePair()/prepareNext()` and
`nextBatch()`. Make the verbose/statistics recording null-safe (check
`queryContext != null` before dereferencing) or ensure a non-null QueryContext
is always provided.
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/query/recent/IoTDBTableFilteredRowsIT.java:
##########
@@ -0,0 +1,200 @@
+package org.apache.iotdb.relational.it.query.recent;
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to you under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * <p>http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * <p>Unless required by applicable law or agreed to in writing, software
distributed under the
+ * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
permissions and
+ * limitations under the License.
+ */
+import org.apache.iotdb.isession.ITableSession;
+import org.apache.iotdb.isession.SessionDataSet;
+import org.apache.iotdb.it.env.EnvFactory;
+import org.apache.iotdb.it.framework.IoTDBTestRunner;
+import org.apache.iotdb.itbase.category.TableClusterIT;
+import org.apache.iotdb.itbase.category.TableLocalStandaloneIT;
+import org.apache.iotdb.rpc.IoTDBConnectionException;
+import org.apache.iotdb.rpc.StatementExecutionException;
+
+import org.apache.tsfile.enums.TSDataType;
+import org.apache.tsfile.write.record.Tablet;
+import org.apache.tsfile.write.schema.IMeasurementSchema;
+import org.apache.tsfile.write.schema.MeasurementSchema;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.runner.RunWith;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import static org.junit.Assert.assertEquals;
+
+@RunWith(IoTDBTestRunner.class)
+@Category({TableLocalStandaloneIT.class, TableClusterIT.class})
+public class IoTDBTableFilteredRowsIT {
+
+ private static final String TABLE_DATABASE = "filter_info";
+ private static final int max_number_of_points_in_page = 10;
+ private static final String DEVICE1 = "device1";
+ private static final List<String> targetKeys =
+ Arrays.asList(
+ "timeSeriesIndexFilteredRows",
+ "chunkIndexFilteredRows",
+ "pageIndexFilteredRows",
+ "rowScanFilteredRows");
Review Comment:
Constant names should follow Java constant conventions (UPPER_SNAKE_CASE).
Rename `max_number_of_points_in_page` (and similarly `targetKeys`) to
`MAX_NUMBER_OF_POINTS_IN_PAGE` / `TARGET_KEYS` (and update references) to avoid
style/checkstyle violations.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/MultiTVListIterator.java:
##########
@@ -235,6 +252,13 @@ public TsBlock nextBatch() {
throw new UnSupportedDataTypeException(
String.format("Data type %s is not supported.", tsDataType));
}
+
+ if (this.getQueryContext().isVerbose() && filteredRowsByPushDownFilter >
0) {
+ this.getQueryContext()
+ .getQueryStatistics()
+ .addFilteredRowsOfRowLevel(filteredRowsByPushDownFilter);
+ }
Review Comment:
`getQueryContext()` may be null (e.g., WritableMemChunk encoding path passes
null into MemPointIteratorFactory), but this code calls
`getQueryContext().isVerbose()` unconditionally, which can cause an NPE. Guard
the verbose/statistics path with a null-check or ensure callers always pass a
non-null QueryContext.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/utils/datastructure/MultiAlignedTVListIterator.java:
##########
@@ -269,13 +274,18 @@ public TsBlock nextBatch() {
}
TsBlock tsBlock = builder.build();
if (pushDownFilter != null) {
+ Consumer<Long> filterRowsRecorder =
+ this.getQueryContext().isVerbose()
+ ? s ->
this.getQueryContext().getQueryStatistics().addFilteredRowsOfRowLevel(s)
+ : null;
Review Comment:
`getQueryContext()` can be null (e.g., iterator used for memchunk
flush/encode is created with null context), but `getQueryContext().isVerbose()`
is called unconditionally here, which can throw a NullPointerException. Make
the recorder creation null-safe (e.g., treat null as non-verbose).
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/memtable/WritableMemChunk.java:
##########
@@ -487,7 +487,7 @@ public void encode(BlockingQueue<Object> ioTaskQueue,
BatchEncodeInfo encodeInfo
tvLists.add(workingListForFlush);
MemPointIterator timeValuePairIterator =
MemPointIteratorFactory.create(
- schema.getType(), tvLists, encodeInfo.maxNumberOfPointsInPage);
+ schema.getType(), tvLists, encodeInfo.maxNumberOfPointsInPage,
null);
Review Comment:
This creates a MemPointIterator with `queryContext` = null, but the updated
iterators now dereference `getQueryContext().isVerbose()` in multiple code
paths (e.g., TVListIterator.prepareNext/nextBatch), which can crash
flushing/encoding with NPE. Pass a non-null QueryContext (with verbose=false)
or update iterators to handle null contexts safely.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/read/reader/chunk/MemPageReader.java:
##########
@@ -137,13 +142,18 @@ public TsBlock getAllSatisfiedData() {
return builder.build();
}
- private boolean[] buildSatisfyInfoArray() {
+ private boolean[] buildSatisfyInfoArray(Consumer<Long> filterRowsRecorder) {
if (recordFilter == null || recordFilter.allSatisfy(this)) {
boolean[] satisfyInfo = new boolean[tsBlock.getPositionCount()];
Arrays.fill(satisfyInfo, true);
return satisfyInfo;
}
- return recordFilter.satisfyTsBlock(tsBlock);
+
+ boolean[] selection = new boolean[tsBlock.getPositionCount()];
+ Arrays.fill(selection, true);
+ return filterRowsRecorder == null
+ ? recordFilter.satisfyTsBlock(tsBlock)
+ : recordFilter.satisfyTsBlock(selection, tsBlock, filterRowsRecorder);
Review Comment:
`buildSatisfyInfoArray` allocates and fills a `selection` array even when
`filterRowsRecorder` is null, but in that branch the array is unused
(`recordFilter.satisfyTsBlock(tsBlock)` is called). Avoid allocating
`selection` unless the recorder is non-null to reduce per-page overhead.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/memtable/AlignedWritableMemChunk.java:
##########
@@ -800,7 +800,8 @@ public void encode(BlockingQueue<Object> ioTaskQueue,
BatchEncodeInfo encodeInfo
columnIndexList,
alignedTvLists,
ignoreAllNullRows,
- encodeInfo.maxNumberOfPointsInPage);
+ encodeInfo.maxNumberOfPointsInPage,
+ null);
Review Comment:
This creates an aligned MemPointIterator with `queryContext` = null. After
the iterator changes, several iterator implementations call
`getQueryContext().isVerbose()` without null-checks, so the encode/flush path
may throw NPE. Pass a non-null QueryContext (verbose=false) or make iterator
code null-safe.
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/storageengine/dataregion/read/reader/chunk/MemAlignedChunkLoaderTest.java:
##########
@@ -116,7 +116,7 @@ public void testMemAlignedChunkLoader() throws IOException {
alignedTvListMap.keySet().stream().map(x -> (AlignedTVList)
x).collect(Collectors.toList());
MemPointIterator timeValuePairIterator =
MemPointIteratorFactory.create(
- dataTypes, null, alignedTvLists, false, maxNumberOfPointsInPage);
+ dataTypes, null, alignedTvLists, false, maxNumberOfPointsInPage,
null);
timeValuePairIterator.setStreamingQueryMemChunk(false);
Review Comment:
This test passes `null` as the QueryContext to MemPointIteratorFactory. With
the new verbose/statistics logic, some iterator implementations call
`getQueryContext().isVerbose()` and will NPE when `nextBatch()` is invoked.
Pass a real QueryContext (e.g., `ctx`) instead of null to keep the test
representative and non-flaky.
##########
integration-test/src/test/java/org/apache/iotdb/relational/it/query/recent/IoTDBTableFilteredRowsIT.java:
##########
@@ -0,0 +1,200 @@
+package org.apache.iotdb.relational.it.query.recent;
+
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for additional
information regarding
+ * copyright ownership. The ASF licenses this file to you under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * <p>http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * <p>Unless required by applicable law or agreed to in writing, software
distributed under the
+ * License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
CONDITIONS OF ANY KIND, either
+ * express or implied. See the License for the specific language governing
permissions and
+ * limitations under the License.
+ */
Review Comment:
The ASF license header should be the first content in the file (before the
`package` declaration). Move the license comment block above the `package` line
to match the repository’s standard header placement and avoid
license/checkstyle issues.
--
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]
