Copilot commented on code in PR #15396:
URL: https://github.com/apache/iotdb/pull/15396#discussion_r2065337508
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##########
@@ -778,141 +809,151 @@ public DataPartition getDataPartition(
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
}
+
+ final Set<TConsensusGroupId> allConsensusGroupIds = new HashSet<>();
+ final Map<TConsensusGroupId, HashSet<TimeSlotRegionInfo>>
consensusGroupToTimeSlotMap =
+ new HashMap<>();
+
Map<String, Map<TSeriesPartitionSlot, Map<TTimePartitionSlot,
List<TRegionReplicaSet>>>>
dataPartitionMap = new HashMap<>();
- // check cache for each database
+
for (Map.Entry<String, List<DataPartitionQueryParam>> entry :
databaseToQueryParamsMap.entrySet()) {
- if (null == entry.getValue()
- || entry.getValue().isEmpty()
- || !getDatabaseDataPartition(dataPartitionMap, entry.getKey(),
entry.getValue())) {
+ String databaseName = entry.getKey();
+ List<DataPartitionQueryParam> params = entry.getValue();
+
+ if (null == params || params.isEmpty()) {
+ cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
+ return null;
+ }
+
+ DataPartitionTable dataPartitionTable =
dataPartitionCache.getIfPresent(databaseName);
+ if (null == dataPartitionTable) {
+ logger.debug(
+ "[{} Cache] miss when search database {}",
+ CacheMetrics.DATA_PARTITION_CACHE_NAME,
+ databaseName);
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
Review Comment:
In the getDatabaseDataPartition method, returning null on a cache miss
instead of false might break the expected boolean contract. Please return false
to maintain consistency.
```suggestion
return false;
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##########
@@ -778,141 +809,151 @@ public DataPartition getDataPartition(
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
}
+
+ final Set<TConsensusGroupId> allConsensusGroupIds = new HashSet<>();
+ final Map<TConsensusGroupId, HashSet<TimeSlotRegionInfo>>
consensusGroupToTimeSlotMap =
+ new HashMap<>();
+
Map<String, Map<TSeriesPartitionSlot, Map<TTimePartitionSlot,
List<TRegionReplicaSet>>>>
dataPartitionMap = new HashMap<>();
- // check cache for each database
+
for (Map.Entry<String, List<DataPartitionQueryParam>> entry :
databaseToQueryParamsMap.entrySet()) {
- if (null == entry.getValue()
- || entry.getValue().isEmpty()
- || !getDatabaseDataPartition(dataPartitionMap, entry.getKey(),
entry.getValue())) {
+ String databaseName = entry.getKey();
+ List<DataPartitionQueryParam> params = entry.getValue();
+
+ if (null == params || params.isEmpty()) {
+ cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
+ return null;
+ }
+
+ DataPartitionTable dataPartitionTable =
dataPartitionCache.getIfPresent(databaseName);
+ if (null == dataPartitionTable) {
+ logger.debug(
+ "[{} Cache] miss when search database {}",
+ CacheMetrics.DATA_PARTITION_CACHE_NAME,
+ databaseName);
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
}
+
+ Map<TSeriesPartitionSlot, SeriesPartitionTable>
cachedDatabasePartitionMap =
+ dataPartitionTable.getDataPartitionMap();
+ Map<TSeriesPartitionSlot, Map<TTimePartitionSlot,
List<TRegionReplicaSet>>>
+ seriesSlotToTimePartitionMap =
+ dataPartitionMap.computeIfAbsent(databaseName, k -> new
HashMap<>());
+
+ for (DataPartitionQueryParam param : params) {
+ TSeriesPartitionSlot seriesPartitionSlot;
+ if (null != param.getDeviceID()) {
+ seriesPartitionSlot =
partitionExecutor.getSeriesPartitionSlot(param.getDeviceID());
+ } else {
+ return null;
Review Comment:
In the getDeviceDataPartition method, returning null when the deviceID is
missing (or when the device lookup fails) disrupts the expected boolean return
type. Please return false instead.
```suggestion
return false;
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##########
@@ -778,141 +809,151 @@ public DataPartition getDataPartition(
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
}
+
+ final Set<TConsensusGroupId> allConsensusGroupIds = new HashSet<>();
+ final Map<TConsensusGroupId, HashSet<TimeSlotRegionInfo>>
consensusGroupToTimeSlotMap =
+ new HashMap<>();
+
Map<String, Map<TSeriesPartitionSlot, Map<TTimePartitionSlot,
List<TRegionReplicaSet>>>>
dataPartitionMap = new HashMap<>();
- // check cache for each database
+
for (Map.Entry<String, List<DataPartitionQueryParam>> entry :
databaseToQueryParamsMap.entrySet()) {
- if (null == entry.getValue()
- || entry.getValue().isEmpty()
- || !getDatabaseDataPartition(dataPartitionMap, entry.getKey(),
entry.getValue())) {
+ String databaseName = entry.getKey();
+ List<DataPartitionQueryParam> params = entry.getValue();
+
+ if (null == params || params.isEmpty()) {
+ cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
+ return null;
+ }
+
+ DataPartitionTable dataPartitionTable =
dataPartitionCache.getIfPresent(databaseName);
+ if (null == dataPartitionTable) {
+ logger.debug(
+ "[{} Cache] miss when search database {}",
+ CacheMetrics.DATA_PARTITION_CACHE_NAME,
+ databaseName);
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
}
+
+ Map<TSeriesPartitionSlot, SeriesPartitionTable>
cachedDatabasePartitionMap =
+ dataPartitionTable.getDataPartitionMap();
+ Map<TSeriesPartitionSlot, Map<TTimePartitionSlot,
List<TRegionReplicaSet>>>
+ seriesSlotToTimePartitionMap =
+ dataPartitionMap.computeIfAbsent(databaseName, k -> new
HashMap<>());
+
+ for (DataPartitionQueryParam param : params) {
+ TSeriesPartitionSlot seriesPartitionSlot;
+ if (null != param.getDeviceID()) {
+ seriesPartitionSlot =
partitionExecutor.getSeriesPartitionSlot(param.getDeviceID());
+ } else {
+ return null;
+ }
+
+ SeriesPartitionTable cachedSeriesPartitionTable =
+ cachedDatabasePartitionMap.get(seriesPartitionSlot);
+ if (null == cachedSeriesPartitionTable) {
+ if (logger.isDebugEnabled()) {
+ logger.debug(
+ "[{} Cache] miss when search device {}",
+ CacheMetrics.DATA_PARTITION_CACHE_NAME,
+ param.getDeviceID());
+ }
+ cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
+ return null;
+ }
+
+ Map<TTimePartitionSlot, List<TConsensusGroupId>>
cachedTimePartitionSlot =
+ cachedSeriesPartitionTable.getSeriesPartitionMap();
+ seriesSlotToTimePartitionMap.computeIfAbsent(seriesPartitionSlot, k
-> new HashMap<>());
+
+ if (param.getTimePartitionSlotList().isEmpty()) {
+ return null;
Review Comment:
In the getDeviceDataPartition method, when the time partition slot list is
empty, returning null violates the method's boolean return expectation.
Returning false would be more appropriate.
```suggestion
return false;
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##########
@@ -778,141 +809,151 @@ public DataPartition getDataPartition(
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
}
+
+ final Set<TConsensusGroupId> allConsensusGroupIds = new HashSet<>();
+ final Map<TConsensusGroupId, HashSet<TimeSlotRegionInfo>>
consensusGroupToTimeSlotMap =
+ new HashMap<>();
+
Map<String, Map<TSeriesPartitionSlot, Map<TTimePartitionSlot,
List<TRegionReplicaSet>>>>
dataPartitionMap = new HashMap<>();
- // check cache for each database
+
for (Map.Entry<String, List<DataPartitionQueryParam>> entry :
databaseToQueryParamsMap.entrySet()) {
- if (null == entry.getValue()
- || entry.getValue().isEmpty()
- || !getDatabaseDataPartition(dataPartitionMap, entry.getKey(),
entry.getValue())) {
+ String databaseName = entry.getKey();
+ List<DataPartitionQueryParam> params = entry.getValue();
+
+ if (null == params || params.isEmpty()) {
+ cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
+ return null;
+ }
+
+ DataPartitionTable dataPartitionTable =
dataPartitionCache.getIfPresent(databaseName);
+ if (null == dataPartitionTable) {
+ logger.debug(
+ "[{} Cache] miss when search database {}",
+ CacheMetrics.DATA_PARTITION_CACHE_NAME,
+ databaseName);
cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
return null;
}
+
+ Map<TSeriesPartitionSlot, SeriesPartitionTable>
cachedDatabasePartitionMap =
+ dataPartitionTable.getDataPartitionMap();
+ Map<TSeriesPartitionSlot, Map<TTimePartitionSlot,
List<TRegionReplicaSet>>>
+ seriesSlotToTimePartitionMap =
+ dataPartitionMap.computeIfAbsent(databaseName, k -> new
HashMap<>());
+
+ for (DataPartitionQueryParam param : params) {
+ TSeriesPartitionSlot seriesPartitionSlot;
+ if (null != param.getDeviceID()) {
+ seriesPartitionSlot =
partitionExecutor.getSeriesPartitionSlot(param.getDeviceID());
+ } else {
+ return null;
+ }
+
+ SeriesPartitionTable cachedSeriesPartitionTable =
+ cachedDatabasePartitionMap.get(seriesPartitionSlot);
+ if (null == cachedSeriesPartitionTable) {
+ if (logger.isDebugEnabled()) {
+ logger.debug(
+ "[{} Cache] miss when search device {}",
+ CacheMetrics.DATA_PARTITION_CACHE_NAME,
+ param.getDeviceID());
+ }
+ cacheMetrics.record(false, CacheMetrics.DATA_PARTITION_CACHE_NAME);
+ return null;
+ }
+
+ Map<TTimePartitionSlot, List<TConsensusGroupId>>
cachedTimePartitionSlot =
+ cachedSeriesPartitionTable.getSeriesPartitionMap();
+ seriesSlotToTimePartitionMap.computeIfAbsent(seriesPartitionSlot, k
-> new HashMap<>());
+
+ if (param.getTimePartitionSlotList().isEmpty()) {
+ return null;
+ }
+
+ for (TTimePartitionSlot timePartitionSlot :
param.getTimePartitionSlotList()) {
+ List<TConsensusGroupId> cacheConsensusGroupIds =
+ cachedTimePartitionSlot.get(timePartitionSlot);
+ if (null == cacheConsensusGroupIds
+ || cacheConsensusGroupIds.isEmpty()
+ || null == timePartitionSlot) {
+ logger.debug(
+ "[{} Cache] miss when search time partition {}",
+ CacheMetrics.DATA_PARTITION_CACHE_NAME,
+ timePartitionSlot);
+ cacheMetrics.record(false,
CacheMetrics.DATA_PARTITION_CACHE_NAME);
+ return null;
Review Comment:
Within the getTimeSlotDataPartition loop, returning null for missing cache
entries instead of false can lead to unintended behavior because the method is
expected to return a boolean value.
```suggestion
return false;
```
--
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: reviews-unsubscr...@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
