JackieTien97 commented on code in PR #16149:
URL: https://github.com/apache/iotdb/pull/16149#discussion_r2272531942
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/fragment/QueryContext.java:
##########
@@ -92,28 +97,62 @@ public QueryContext(long queryId, boolean debug, long
startTime, long timeout) {
// if the mods file does not exist, do not add it to the cache
protected boolean checkIfModificationExists(TsFileResource tsFileResource) {
- if (nonExistentModFiles.contains(tsFileResource.getTsFileID())) {
- return false;
- }
+ // The exists state of ModificationFile is maintained in memory, and
ModificationFile instance
+ // is set to the related TsFileResource instance after it is constructed.
+ return tsFileResource.anyModFileExists();
+ }
- if (!tsFileResource.anyModFileExists()) {
- nonExistentModFiles.add(tsFileResource.getTsFileID());
- return false;
+ private PatternTreeMap<ModEntry, ModsSerializer>
getAllModifications(TsFileResource resource) {
+ if (!(this instanceof FragmentInstanceContext)) {
+ return fileModCache.computeIfAbsent(
+ resource.getTsFileID(), k -> loadAllModificationsFromDisk(resource));
}
Review Comment:
if so, why not override getAllModifications in `FragmentInstanceContext`.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/compaction/selector/estimator/CompactionEstimateUtils.java:
##########
@@ -167,7 +167,7 @@ static CompactionTaskMetadataInfo
collectMetadataInfoFromDisk(
Map<IDeviceID, Long> deviceMetadataSizeMap = new HashMap<>();
try {
for (TsFileResource resource : resources) {
- cost += resource.getTotalModSizeInByte();
+ cost += CompactionTaskInfo.getMemCostForCachedModEntries(resource);
Review Comment:
Won't this calculation be too heavy for compaction? Since it will
deserialize all the mods from disk
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/execution/fragment/QueryContext.java:
##########
@@ -41,23 +44,27 @@
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
import java.util.stream.Collectors;
/** QueryContext contains the shared information with in a query. */
public class QueryContext {
private static final Logger LOGGER =
LoggerFactory.getLogger(QueryContext.class);
+ protected static IoTDBConfig config =
IoTDBDescriptor.getInstance().getConfig();
Review Comment:
```suggestion
private static final IoTDBConfig CONFIG =
IoTDBDescriptor.getInstance().getConfig();
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java:
##########
@@ -1110,6 +1112,23 @@ public static long
calculateDefaultSortBufferSize(DataNodeMemoryConfig memoryCon
/ 2);
}
+ private void loadModsCacheSizeLimitPerFI(TrimProperties properties) {
+ long defaultValue =
+ Math.min(
+ 32 * 1024 * 1024L,
+
memoryConfig.getOperatorsMemoryManager().getTotalMemorySizeInBytes()
+ / memoryConfig.getQueryThreadCount()
+ / 2);
+ long size =
+ Long.parseLong(
+ properties.getProperty(
+ "mods_cache_size_limit_per_fi_in_bytes",
Long.toString(defaultValue)));
+ if (size <= 0) {
+ size = defaultValue;
+ }
+ conf.setModsCacheSizeLimitPerFI(size);
+ }
Review Comment:
extract the same calculation logic from sor_buffer_size
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/modification/ModEntry.java:
##########
@@ -177,7 +179,10 @@ public static ModType deserialize(ByteBuffer buffer) {
}
public static ModType deserialize(InputStream stream) throws IOException {
- byte typeNum = ReadWriteIOUtils.readByte(stream);
+ int typeNum = stream.read();
+ if (typeNum == -1) {
+ throw new EOFException();
+ }
Review Comment:
Why we need this specific `EOFException`?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/dataregion/modification/TreeDeletionEntry.java:
##########
@@ -114,12 +120,21 @@ public void deserialize(InputStream stream) throws
IOException {
public void deserialize(ByteBuffer buffer) {
super.deserialize(buffer);
try {
- this.pathPattern = new
MeasurementPath(ReadWriteIOUtils.readVarIntString(buffer));
+ this.pathPattern =
getMeasurementPath(ReadWriteIOUtils.readVarIntString(buffer));
} catch (IllegalPathException e) {
throw new IllegalArgumentException(e);
}
}
+ private MeasurementPath getMeasurementPath(String path) throws
IllegalPathException {
+ if (path.contains(TsFileConstant.BACK_QUOTE_STRING)) {
+ return new MeasurementPath(PathUtils.splitPathToDetachedNodes(path));
+ } else {
+ String[] nodes = path.split(TsFileConstant.PATH_SEPARATER_NO_REGEX);
+ return new MeasurementPath(nodes);
+ }
+ }
+
Review Comment:
add some comments about this function. You want to avoid time-consuming
antlr parse here for those path which doesn't contain special characters?
--
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]
