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/09/18 05:45:07 UTC
[GitHub] [incubator-iotdb] liutaohua commented on a change in pull request #1731: [IOTDB-870] tags and attributes output
liutaohua commented on a change in pull request #1731:
URL: https://github.com/apache/incubator-iotdb/pull/1731#discussion_r490696999
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/MManager.java
##########
@@ -994,14 +993,13 @@ private boolean match(PartialPath fullPath, String[] prefixNodes) {
if (tagFileOffset < 0) {
// no tags/attributes
res.add(new ShowTimeSeriesResult(ansString.left.getFullPath(), ansString.right[0], ansString.right[1], ansString.right[2],
- ansString.right[3], ansString.right[4], Collections.emptyMap()));
+ ansString.right[3], ansString.right[4], Collections.emptyMap(), Collections.emptyMap()));
} else {
// has tags/attributes
Pair<Map<String, String>, Map<String, String>> pair =
tagLogFile.read(config.getTagAttributeTotalSize(), tagFileOffset);
- pair.left.putAll(pair.right);
res.add(new ShowTimeSeriesResult(ansString.left.getFullPath(), ansString.right[0], ansString.right[1], ansString.right[2],
- ansString.right[3], ansString.right[4], pair.left));
+ ansString.right[3], ansString.right[4], pair.left, pair.right));
Review comment:
how about:
```
Pair<Map<String, String>, Map<String, String>> pair = new Pair(Collections.emptyMap(),Collections.emptyMap());
if(tagFileOffset >= 0){
pair = tagLogFile.read(config.getTagAttributeTotalSize(), tagFileOffset);
}
res.add(new ShowTimeSeriesResult(ansString.left.getFullPath(), ansString.right[0], ansString.right[1], ansString.right[2], pair.left, pair.right);
```
##########
File path: server/src/main/java/org/apache/iotdb/db/query/dataset/ShowTimeSeriesResult.java
##########
@@ -113,10 +119,21 @@ public void serialize(OutputStream outputStream) throws IOException {
ReadWriteIOUtils.write(encoding, outputStream);
ReadWriteIOUtils.write(compressor, outputStream);
- ReadWriteIOUtils.write(tagAndAttribute != null, outputStream); //flag
- if (tagAndAttribute != null) {
- ReadWriteIOUtils.write(tagAndAttribute.size(), outputStream);
- for (Entry<String, String> stringStringEntry : tagAndAttribute.entrySet()) {
+ //flag for tag
+ ReadWriteIOUtils.write(tags != null, outputStream);
+ if (tags != null) {
+ ReadWriteIOUtils.write(tags.size(), outputStream);
+ for (Entry<String, String> stringStringEntry : tags.entrySet()) {
+ ReadWriteIOUtils.write(stringStringEntry.getKey(), outputStream);
+ ReadWriteIOUtils.write(stringStringEntry.getValue(), outputStream);
+ }
+ }
+
+ //flag for attribute
+ ReadWriteIOUtils.write(attributes != null, outputStream);
Review comment:
tags and attr are duplicate code, how about:
```
private void writeNullable(Map<String,String> param, OutputStream out){
ReadWriteIOUtils.write(param != null, outputStream);
if (param != null) {
ReadWriteIOUtils.write(tags.size(), outputStream);
for (Entry<String, String> entry : param.entrySet()) {
ReadWriteIOUtils.write(entry.getKey(), outputStream);
ReadWriteIOUtils.write(entry.getValue(), outputStream);
}
}
}
```
##########
File path: server/src/main/java/org/apache/iotdb/db/query/dataset/ShowTimeSeriesResult.java
##########
@@ -134,13 +151,25 @@ public static ShowTimeSeriesResult deserialize(ByteBuffer buffer) {
result.encoding = ReadWriteIOUtils.readString(buffer);
result.compressor = ReadWriteIOUtils.readString(buffer);
Review comment:
we have the enums of these strings, I suggest that we use the enums to serialize, u can find it in :
```
package org.apache.iotdb.tsfile.file.metadata.enums;
```
##########
File path: server/src/main/java/org/apache/iotdb/db/utils/QueryUtils.java
##########
@@ -127,12 +128,27 @@ public static void constructPathAndDataTypes(List<PartialPath> paths, List<TSDat
paths.add(new PartialPath(COLUMN_TIMESERIES_COMPRESSION, false));
dataTypes.add(TSDataType.TEXT);
Review comment:
duplicate code, how about:
```
private static PartialPath[] resourcePaths = new PartialPath[]{new PartialPath(COLUMN_TIMESERIES, false), .....};
private static TSDataType[] resourceTypes = new TSDataType[]{TSDataType.TEXT, .....};
public static void constructPathAndDataTypes(List<PartialPath> paths, List<TSDataType> dataTypes, List<ShowTimeSeriesResult> timeseriesList) {
Collections.addAll(paths,resourcePaths);
Collections.addAll(dataTypes,resourceTypes);
......
}
```
##########
File path: server/src/main/java/org/apache/iotdb/db/utils/QueryUtils.java
##########
@@ -127,12 +128,27 @@ public static void constructPathAndDataTypes(List<PartialPath> paths, List<TSDat
paths.add(new PartialPath(COLUMN_TIMESERIES_COMPRESSION, false));
dataTypes.add(TSDataType.TEXT);
- Set<String> tagAndAttributeName = new TreeSet<>();
+ //check if timeseries result has tag or attribute
+ boolean hasTag = false;
+ boolean hasAttribute = false;
for (ShowTimeSeriesResult result : timeseriesList) {
Review comment:
I don't think there's any need to check, it's ok to show `null || spaces`, like :
```
+--------------+-----+-------------+--------+--------+-----------+----+----------+
| timeseries|alias|storage group|dataType|encoding|compression|tags|attributes|
+--------------+-----+-------------+--------+--------+-----------+----+----------+
|root.sg1.d1.s1| null| root.sg1| INT64| RLE| SNAPPY|null| null|
|root.sg1.d1.s2| null| root.sg1| INT64| RLE| SNAPPY|null| null|
|root.sg1.d1.s3| null| root.sg1| INT64| RLE| SNAPPY|null| null|
+--------------+-----+-------------+--------+--------+-----------+----+----------+
```
----------------------------------------------------------------
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