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 2021/02/22 15:21:47 UTC
[GitHub] [iotdb] Alima777 commented on a change in pull request #2714: [IOTDB-1024] Support multiple aggregated measurements for group by level statement
Alima777 commented on a change in pull request #2714:
URL: https://github.com/apache/iotdb/pull/2714#discussion_r580302080
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/aggregation/IoTDBAggregationByLevelIT.java
##########
@@ -59,6 +59,10 @@
};
private static final double DOUBLE_PRECISION = 0.001d;
+ private final String sg1d1 = "root.sg1.d1";
+ private final String sg1d2 = "root.sg1.d2";
+ private final String sg2d1 = "root.sg2.d1";
+ private final String sg2d2 = "root.sg2.d2";
Review comment:
Remove this which is never used.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/physical/crud/AggregationPlan.java
##########
@@ -67,4 +77,25 @@ public int getLevel() {
public void setLevel(int level) {
this.level = level;
}
+
+ public Map<String, AggregateResult> getPathByLevel() throws QueryProcessException {
+ if (!finalAggreagtionPaths.isEmpty()) {
+ return finalAggreagtionPaths;
+ }
+ List<PartialPath> seriesPaths = getPaths();
+ List<TSDataType> dataTypes = getDataTypes();
+ try {
+ for (int i = 0; i < seriesPaths.size(); i++) {
+ String transformedPath =
+ FilePathUtils.generatePartialPathByLevel(seriesPaths.get(i).getFullPath(), getLevel());
+ String key = getAggregations().get(i) + "(" + transformedPath + ")";
+ AggregateResult aggRet =
+ AggregateResultFactory.getAggrResultByName(getAggregations().get(i), dataTypes.get(i));
+ finalAggreagtionPaths.putIfAbsent(key, aggRet);
Review comment:
Generate the AggrResult every time is unnecessary.
```suggestion
if (finalAggreagtionPaths.containsKey(key)) {
AggregateResult aggRet =
AggregateResultFactory.getAggrResultByName(getAggregations().get(i), dataTypes.get(i));
finalAggreagtionPaths.put(key, aggRet);
}
```
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/aggregation/IoTDBAggregationByLevelIT.java
##########
@@ -191,26 +204,35 @@ public void timeFuncGroupByLevelTest() throws Exception {
public void valueFuncGroupByLevelTest() throws Exception {
String[] retArray =
new String[] {
- "61.22", "71.12,62.15",
+ "61.22,125.5", "71.12,62.15,71.12,62.15",
};
try (Connection connection =
DriverManager.getConnection("jdbc:iotdb://127.0.0.1:6667/", "root", "root");
Statement statement = connection.createStatement()) {
- statement.execute("select last_value(temperature) from root.*.* GROUP BY level=0");
+ statement.execute(
+ "select last_value(temperature),max_value(temperature) from root.*.* GROUP BY level=0");
int cnt = 0;
try (ResultSet resultSet = statement.getResultSet()) {
while (resultSet.next()) {
- String ans = resultSet.getString(1);
+ String ans = resultSet.getString(1) + "," + resultSet.getString(2);
Assert.assertEquals(retArray[cnt], ans);
cnt++;
}
}
- statement.execute("select max_value(temperature) from root.sg1.* GROUP BY level=2");
+ statement.execute(
+ "select last_value(temperature),max_value(temperature) from root.sg1.* GROUP BY level=2");
Review comment:
```suggestion
"select last_value(temperature), max_value(temperature) from root.sg1.* GROUP BY level=2");
```
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/aggregation/IoTDBAggregationByLevelIT.java
##########
@@ -159,26 +163,35 @@ public void avgFuncGroupByLevelTest() throws Exception {
public void timeFuncGroupByLevelTest() throws Exception {
String[] retArray =
new String[] {
- "100", "600,700",
+ "8,100", "600,700,2,3",
};
try (Connection connection =
DriverManager.getConnection("jdbc:iotdb://127.0.0.1:6667/", "root", "root");
Statement statement = connection.createStatement()) {
- statement.execute("select min_time(temperature) from root.*.* GROUP BY level=0");
+ statement.execute(
+ "select count(status), min_time(temperature) from root.*.* GROUP BY level=0");
int cnt = 0;
try (ResultSet resultSet = statement.getResultSet()) {
while (resultSet.next()) {
- String ans = resultSet.getString(1);
+ String ans = resultSet.getString(1) + "," + resultSet.getString(2);
Assert.assertEquals(retArray[cnt], ans);
cnt++;
}
}
Review comment:
Check the metadata result(column name) concurrently because you modified this. And try to check it using different value of level in test.
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/strategy/PhysicalGenerator.java
##########
@@ -876,6 +877,16 @@ private void deduplicate(QueryPlan queryPlan, int fetchSize)
.getContext()
.getColumnName()
: columnForReader;
+ if (queryPlan instanceof AggregationPlan && ((AggregationPlan) queryPlan).getLevel() >= 0) {
+ String aggregatePath =
+ originalPath.isMeasurementAliasExists()
+ ? FilePathUtils.generatePartialPathByLevel(
+ originalPath.getFullPathWithAlias(), ((AggregationPlan) queryPlan).getLevel())
+ : FilePathUtils.generatePartialPathByLevel(
+ originalPath.toString(), ((AggregationPlan) queryPlan).getLevel());
Review comment:
In line 850, check the level and don't change the value of `columnForReader`. In this way, you can use the `columnForReader` directly here.
##########
File path: server/src/test/java/org/apache/iotdb/db/integration/aggregation/IoTDBAggregationByLevelIT.java
##########
@@ -191,26 +204,35 @@ public void timeFuncGroupByLevelTest() throws Exception {
public void valueFuncGroupByLevelTest() throws Exception {
String[] retArray =
new String[] {
- "61.22", "71.12,62.15",
+ "61.22,125.5", "71.12,62.15,71.12,62.15",
};
try (Connection connection =
DriverManager.getConnection("jdbc:iotdb://127.0.0.1:6667/", "root", "root");
Statement statement = connection.createStatement()) {
- statement.execute("select last_value(temperature) from root.*.* GROUP BY level=0");
+ statement.execute(
+ "select last_value(temperature),max_value(temperature) from root.*.* GROUP BY level=0");
Review comment:
```suggestion
"select last_value(temperature), max_value(temperature) from root.*.* GROUP BY level=0");
```
##########
File path: server/src/main/java/org/apache/iotdb/db/utils/FilePathUtils.java
##########
@@ -195,43 +171,49 @@ public static String getTsFilePrefixPath(TsFileResource resource) {
if (newRecord.getFields().get(i) == null) {
aggregateResultList.add(
AggregateResultFactory.getAggrResultByName(
- plan.getAggregations().get(i), plan.getDeduplicatedDataTypes().get(i)));
+ plan.getDeduplicatedAggregations().get(i), plan.getDeduplicatedDataTypes().get(i)));
} else {
TSDataType dataType = newRecord.getFields().get(i).getDataType();
AggregateResult aggRet =
- AggregateResultFactory.getAggrResultByName(plan.getAggregations().get(i), dataType);
- switch (dataType) {
- case TEXT:
- aggRet.setBinaryValue(newRecord.getFields().get(i).getBinaryV());
- break;
- case INT32:
- aggRet.setIntValue(newRecord.getFields().get(i).getIntV());
- break;
- case INT64:
- aggRet.setLongValue(newRecord.getFields().get(i).getLongV());
- break;
- case FLOAT:
- aggRet.setFloatValue(newRecord.getFields().get(i).getFloatV());
- break;
- case DOUBLE:
- aggRet.setDoubleValue(newRecord.getFields().get(i).getDoubleV());
- break;
- case BOOLEAN:
- aggRet.setBooleanValue(newRecord.getFields().get(i).getBoolV());
- break;
- default:
- throw new UnSupportedDataTypeException(dataType.toString());
+ AggregateResultFactory.getAggrResultByName(
+ plan.getDeduplicatedAggregations().get(i), dataType);
+ if (aggRet.getAggregationType().equals(AggregationType.AVG)) {
+ ((AvgAggrResult) aggRet).updateAvg(dataType, newRecord.getFields().get(i).getDoubleV());
+ } else {
Review comment:
If it's just for here, I think it's better to write a new public method `setAvg`. which set `avg` and `cnt` directly rather than modify the access restrictions of `private` method.
----------------------------------------------------------------
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