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/07/29 07:51:49 UTC
[GitHub] [incubator-iotdb] JackieTien97 commented on a change in pull request #1496: Optimize path
JackieTien97 commented on a change in pull request #1496:
URL: https://github.com/apache/incubator-iotdb/pull/1496#discussion_r461482285
##########
File path: tsfile/src/main/java/org/apache/iotdb/tsfile/read/common/Path.java
##########
@@ -165,50 +156,121 @@ public static Path addPrefixPath(Path src, Path prefix) {
return addPrefixPath(src, prefix.fullPath);
}
+ public void setDevice(String device) {
+ this.device = device;
+ }
+
+ public void setFullPath(String fullPath) {
+ this.fullPath = fullPath;
+ }
+
public String getFullPath() {
- return fullPath;
+ if(fullPath != null) {
+ return fullPath;
+ } else {
+ StringBuilder s = new StringBuilder(nodes.get(0));
+ for(int i = 1; i < nodes.size(); i++) {
+ s.append(TsFileConstant.PATH_SEPARATOR);
+ s.append(nodes.get(i));
+ }
+ fullPath = s.toString();
+ return fullPath;
+ }
}
public String getDevice() {
- return device;
+ if(device != null) {
+ return device;
+ } else {
+ StringBuilder s = new StringBuilder(nodes.get(0));
+ for(int i = 1; i < nodes.size() - 1; i++) {
+ s.append(TsFileConstant.PATH_SEPARATOR);
+ s.append(nodes.get(i));
+ }
+ device = s.toString();
+ return device;
+ }
}
public String getMeasurement() {
- return measurement;
+ if(measurement != null) {
+ return measurement;
+ } else {
+ measurement = nodes.get(nodes.size() - 1);
+ return measurement;
+ }
}
public String getAlias() { return alias; }
+ public List<String> getNodes() {
+ return nodes;
+ }
+
public void setAlias(String alias) { this.alias = alias; }
- public String getFullPathWithAlias() { return device + TsFileConstant.PATH_SEPARATOR + alias; }
+ public String getFullPathWithAlias() {
+ if(device != null) {
+ return device + TsFileConstant.PATH_SEPARATOR + alias;
+ } else {
+ this.device = getDevice();
+ return device + TsFileConstant.PATH_SEPARATOR + alias;
+ }
+ }
@Override
public int hashCode() {
- return fullPath.hashCode();
+ if(fullPath != null) {
+ return fullPath.hashCode();
+ } else {
+ fullPath = getFullPath();
+ return fullPath.hashCode();
+ }
}
@Override
public boolean equals(Object obj) {
- return obj instanceof Path && this.fullPath.equals(((Path) obj).fullPath);
+ if(obj instanceof Path) {
+ if(fullPath != null) {
+ return fullPath.equals(((Path) obj).getFullPath());
+ } else {
+ for(int i = 0; i < nodes.size(); i++) {
+ if(!nodes.get(0).equals(((Path) obj).nodes.get(0))) {
Review comment:
```suggestion
if(!nodes.get(i).equals(((Path) obj).nodes.get(i))) {
```
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -286,9 +288,15 @@ public void deserialize() throws IOException {
String path = ReadWriteIOUtils.readString(inputStream);
long time = ReadWriteIOUtils.readLong(inputStream);
// To reduce the String number in memory,
- // use the deviceId from MManager instead of the deviceId read from disk
- path = IoTDB.metaManager.getDeviceId(path);
- deviceMap.put(path, i);
+ // use the deviceId from memory instead of the deviceId read from disk
+ if (devicePool == null) {
+ devicePool = new HashMap<>();
+ }
+ String tempPath = devicePool.putIfAbsent(path, path);
+ if (tempPath == null) {
+ tempPath = path;
+ }
+ deviceMap.put(tempPath, i);
Review comment:
```suggestion
String tempPath = devicePool.get(path);
if (tempPath == null) {
devicePool.put(path, path);
tempPath = path;
}
deviceMap.put(tempPath, i);
```
No need to do the duplicated check in putIfAbsent()
##########
File path: tsfile/src/main/java/org/apache/iotdb/tsfile/read/common/Path.java
##########
@@ -72,8 +74,7 @@ public Path(String device, String measurement) {
}
this.device = device;
this.measurement = measurement;
- this.fullPath = device + TsFileConstant.PATH_SEPARATOR
- + (measurement.contains(TsFileConstant.PATH_SEPARATOR) ? "\"" + measurement + "\"" : measurement);
+ this.fullPath = device + TsFileConstant.PATH_SEPARATOR + measurement;
Review comment:
Why do changes here? We don't need to consider the measurement name that contains '.'?
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/MetaUtils.java
##########
@@ -75,4 +115,39 @@ public static String getStorageGroupNameByLevel(String path, int level) throws M
}
return storageGroupName.toString();
}
+
+ /**
+ * Get storage group name when creating schema automatically is enable
+ *
+ * e.g., nodes = [root, a, b, c] and level = 1, return [root, a]
+ *
+ * @param nodeNames nodeNames
+ * @param level level
+ */
+ public static List<String> getStorageGroupNameNodesByLevel(List<String> nodeNames, int level) throws MetadataException {
+ if (nodeNames.size() <= level || !nodeNames.get(0).equals(IoTDBConstant.PATH_ROOT)) {
+ throw new IllegalPathException(getPathByNodes(nodeNames));
+ }
+ List<String> storageGroup = new ArrayList<>(nodeNames);
+ for(int i = storageGroup.size() - 1; storageGroup.size() - 1 > level; i--) {
+ storageGroup.remove(i);
+ }
+ return storageGroup;
Review comment:
```suggestion
return new ArrayList<>(nodeNames.subList(0, level+1));
```
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/MetaUtils.java
##########
@@ -75,4 +115,39 @@ public static String getStorageGroupNameByLevel(String path, int level) throws M
}
return storageGroupName.toString();
}
+
+ /**
+ * Get storage group name when creating schema automatically is enable
+ *
+ * e.g., nodes = [root, a, b, c] and level = 1, return [root, a]
+ *
+ * @param nodeNames nodeNames
+ * @param level level
+ */
+ public static List<String> getStorageGroupNameNodesByLevel(List<String> nodeNames, int level) throws MetadataException {
+ if (nodeNames.size() <= level || !nodeNames.get(0).equals(IoTDBConstant.PATH_ROOT)) {
+ throw new IllegalPathException(getPathByNodes(nodeNames));
+ }
+ List<String> storageGroup = new ArrayList<>(nodeNames);
+ for(int i = storageGroup.size() - 1; storageGroup.size() - 1 > level; i--) {
+ storageGroup.remove(i);
+ }
+ return storageGroup;
+ }
+
+ /**
+ * Combine a path by a string list
+ * e.g., nodes = [root, a, b, c], return root.a.b.c
+ */
+
+ public static String getPathByNodes(List<String> nodes) {
+ StringBuilder path = new StringBuilder();
+ path.append(nodes.get(0));
+ for(int i = 1; i < nodes.size(); i++) {
+ path.append(TsFileConstant.PATH_SEPARATOR);
+ path.append(nodes.get(i));
+ }
+ return path.toString();
Review comment:
```suggestion
return String.join(TsFileConstant.PATH_SEPARATOR, nodes);
```
##########
File path: tsfile/src/main/java/org/apache/iotdb/tsfile/read/common/Path.java
##########
@@ -165,50 +156,121 @@ public static Path addPrefixPath(Path src, Path prefix) {
return addPrefixPath(src, prefix.fullPath);
}
+ public void setDevice(String device) {
+ this.device = device;
+ }
+
+ public void setFullPath(String fullPath) {
+ this.fullPath = fullPath;
+ }
+
public String getFullPath() {
- return fullPath;
+ if(fullPath != null) {
+ return fullPath;
+ } else {
+ StringBuilder s = new StringBuilder(nodes.get(0));
+ for(int i = 1; i < nodes.size(); i++) {
+ s.append(TsFileConstant.PATH_SEPARATOR);
+ s.append(nodes.get(i));
+ }
+ fullPath = s.toString();
+ return fullPath;
+ }
}
public String getDevice() {
- return device;
+ if(device != null) {
+ return device;
+ } else {
+ StringBuilder s = new StringBuilder(nodes.get(0));
+ for(int i = 1; i < nodes.size() - 1; i++) {
+ s.append(TsFileConstant.PATH_SEPARATOR);
+ s.append(nodes.get(i));
+ }
+ device = s.toString();
+ return device;
Review comment:
same as above
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/MTree.java
##########
@@ -511,6 +578,52 @@ String getStorageGroupName(String path) throws StorageGroupNotSetException {
throw new StorageGroupNotSetException(path);
}
+ /**
+ * Get storage group name by path
+ *
+ * <p>e.g., root.sg1 is storage group, path is root.sg1.d1, return root.sg1
+ *
+ * @return storage group in the given path
+ */
+ String getStorageGroupName(List<String> nodes) throws StorageGroupNotSetException {
+ MNode cur = root;
+ for (int i = 1; i < nodes.size(); i++) {
+ cur = cur.getChild(nodes.get(i));
+ if (cur instanceof StorageGroupMNode) {
+ return cur.getFullPath();
+ } else if (cur == null) {
+ throw new StorageGroupNotSetException(MetaUtils.getPathByNodes(nodes));
+ }
+ }
+ throw new StorageGroupNotSetException(MetaUtils.getPathByNodes(nodes));
+ }
+
+ /**
+ * Get storage group name by nodes
+ *
+ * <p>e.g., root.sg1 is storage group, nodes is [root, sg1, d1], return [root, sg1]
+ *
+ * @return storage group in the given nodes
+ */
+ List<String> getStorageGroupNodes(List<String> nodes) throws StorageGroupNotSetException {
+ MNode cur = root;
+ for (int i = 1; i < nodes.size(); i++) {
+ cur = cur.getChild(nodes.get(i));
+ if (cur instanceof StorageGroupMNode) {
+ break;
+ } else if (cur == null) {
+ throw new StorageGroupNotSetException(MetaUtils.getPathByNodes(nodes));
+ }
+ }
+ List<String> storageGroupNodes = new ArrayList<>();
+ storageGroupNodes.add(0, cur.getName());
+ while(cur.getParent() != null) {
+ cur = cur.getParent();
+ storageGroupNodes.add(0, cur.getName());
+ }
+ return storageGroupNodes;
Review comment:
Why not contruct storageGroupNodes list in the first check loop
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/MManager.java
##########
@@ -157,22 +157,7 @@ protected MManager() {
isRecovering = true;
int cacheSize = config.getmManagerCacheSize();
- mNodeCache = new RandomDeleteCache<String, MNode>(cacheSize) {
-
- @Override
- public MNode loadObjectByKey(String key) throws CacheException {
- lock.readLock().lock();
- try {
- return mtree.getNodeByPathWithStorageGroupCheck(key);
- } catch (MetadataException e) {
- throw new CacheException(e);
- } finally {
- lock.readLock().unlock();
- }
- }
- };
-
- int remoteCacheSize = config.getmRemoteSchemaCacheSize();
+ mNodeCache = new RandomDeleteCache<String, MNode>(cacheSize) {};
Review comment:
If so, don't need to declare RandomDeleteCache as abstract class
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/MTree.java
##########
@@ -792,6 +1192,23 @@ static long getLastTimeStamp(MeasurementMNode node, QueryContext queryContext) {
return childNodePaths;
}
+ /**
+ * Get child node path in the next level of the given path.
+ *
+ * <p>e.g., MTree has [root.sg1.d1.s1, root.sg1.d1.s2, root.sg1.d2.s1] given path = root.sg1,
+ * return [root.sg1.d1, root.sg1.d2]
+ *
+ * @return All child nodes' seriesPath(s) of given seriesPath.
+ */
+ Set<String> getChildNodePathInNextLevel(List<String> nodes) throws MetadataException {
+ if (nodes.isEmpty() || !nodes.get(0).equals(root.getName())) {
+ throw new IllegalPathException(MetaUtils.getPathByNodes(nodes));
+ }
+ Set<String> childNodePaths = new TreeSet<>();
Review comment:
Why use TreeSet, we need to keep that in alphabetical sequence?
##########
File path: tsfile/src/main/java/org/apache/iotdb/tsfile/read/common/Path.java
##########
@@ -165,50 +156,121 @@ public static Path addPrefixPath(Path src, Path prefix) {
return addPrefixPath(src, prefix.fullPath);
}
+ public void setDevice(String device) {
+ this.device = device;
+ }
+
+ public void setFullPath(String fullPath) {
+ this.fullPath = fullPath;
+ }
+
public String getFullPath() {
- return fullPath;
+ if(fullPath != null) {
+ return fullPath;
+ } else {
+ StringBuilder s = new StringBuilder(nodes.get(0));
+ for(int i = 1; i < nodes.size(); i++) {
+ s.append(TsFileConstant.PATH_SEPARATOR);
+ s.append(nodes.get(i));
+ }
+ fullPath = s.toString();
+ return fullPath;
Review comment:
You can call the method in MetaUtil.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -653,15 +653,15 @@ private void addRowRecordForShowQuery(
@Override
public void delete(DeletePlan deletePlan) throws QueryProcessException {
try {
- Set<String> existingPaths = new HashSet<>();
+ Set<List<String>> existingPaths = new HashSet<>();
for (Path p : deletePlan.getPaths()) {
- existingPaths.addAll(getPathsName(p.getFullPath()));
+ existingPaths.addAll(getPathsName(p.getNodes()));
Review comment:
Set<List<String>> can't be used directly.
List<String> a = List.of("a", "b").
List<String> b = List.of ("a", "b").
a is not equal to b.
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/MetaUtils.java
##########
@@ -56,6 +64,38 @@ static String getNodeRegByIdx(int idx, String[] nodes) {
return idx >= nodes.length ? PATH_WILDCARD : nodes[idx];
}
+ public static List<String> getDeviceNodeNames(String path) {
+ List<String> deviceNodes = new ArrayList<>();
+ Collections.addAll(deviceNodes, path.split(PATH_SEPARATOR));
+ return deviceNodes;
+ }
+
+ public static List<String> splitPathToNodes(String path) throws IllegalPathException {
+ List<String> nodes = new ArrayList<>();
+ int startIndex = 0;
+ for (int i = 0; i < path.length(); i++) {
+ if (path.charAt(i) == '.') {
Review comment:
. should be replaced with PATH_SEPARATOR
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -286,9 +288,15 @@ public void deserialize() throws IOException {
String path = ReadWriteIOUtils.readString(inputStream);
long time = ReadWriteIOUtils.readLong(inputStream);
// To reduce the String number in memory,
- // use the deviceId from MManager instead of the deviceId read from disk
- path = IoTDB.metaManager.getDeviceId(path);
- deviceMap.put(path, i);
+ // use the deviceId from memory instead of the deviceId read from disk
+ if (devicePool == null) {
+ devicePool = new HashMap<>();
+ }
Review comment:
No need to do the check each time. Init it where it's declared.
----------------------------------------------------------------
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