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/05/20 01:38:43 UTC
[GitHub] [incubator-iotdb] Ring-k opened a new pull request #1230: [IOTDB-665] validate physical plan before fowarding
Ring-k opened a new pull request #1230:
URL: https://github.com/apache/incubator-iotdb/pull/1230
This pull request implements physical plan validation before fowarding. The client connects and sends requests to a node in the cluster, which should response some messages if the statement is invalid.
----------------------------------------------------------------
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
[GitHub] [incubator-iotdb] jt2594838 commented on a change in pull request #1230: [IOTDB-665] validate physical plan before fowarding
Posted by GitBox <gi...@apache.org>.
jt2594838 commented on a change in pull request #1230:
URL: https://github.com/apache/incubator-iotdb/pull/1230#discussion_r429859587
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1397,6 +1401,91 @@ public TSStatus executeNonQuery(PhysicalPlan plan) {
}
}
+
+ public TSStatus validatePlan(PhysicalPlan plan) {
+ TSStatus result = StatusUtils.OK;
+ if (plan instanceof SetStorageGroupPlan) {
+ return validateSetStorageGroupPlan((SetStorageGroupPlan) plan);
+ } else if (plan instanceof InsertPlan) {
+ return validateInsertPlan((InsertPlan) plan);
+ }
+ // TODO add more pre-validations
+ return result;
+ }
+
+ private TSStatus validateSetStorageGroupPlan(SetStorageGroupPlan plan) {
+ String path = plan.getPath().getFullPath();
+ if (getAllStorageGroupNames().contains(path)) {
+ return StatusUtils.PATH_ALREADY_EXIST_ERROR;
+ }
+ return StatusUtils.OK;
+ }
+
+ private TSStatus validateInsertPlan(InsertPlan plan) {
+ List<Path> paths = (plan).getPaths();
+ String[] values = (plan).getValues();
+ List<String> pathStrings = new ArrayList<>();
+ for (Path path : paths) {
+ pathStrings.add(path.getFullPath());
+ }
+ List<MeasurementSchema> measurementSchemas;
+ try {
+ measurementSchemas = pullTimeSeriesSchemas(pathStrings);
Review comment:
Check the schema cache in MManager using MManager.getSeriesType() first, if it cannot be found then add them to the list and pull them.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1397,6 +1401,91 @@ public TSStatus executeNonQuery(PhysicalPlan plan) {
}
}
+
+ public TSStatus validatePlan(PhysicalPlan plan) {
+ TSStatus result = StatusUtils.OK;
+ if (plan instanceof SetStorageGroupPlan) {
+ return validateSetStorageGroupPlan((SetStorageGroupPlan) plan);
+ } else if (plan instanceof InsertPlan) {
+ return validateInsertPlan((InsertPlan) plan);
+ }
+ // TODO add more pre-validations
+ return result;
+ }
+
+ private TSStatus validateSetStorageGroupPlan(SetStorageGroupPlan plan) {
+ String path = plan.getPath().getFullPath();
+ if (getAllStorageGroupNames().contains(path)) {
+ return StatusUtils.PATH_ALREADY_EXIST_ERROR;
+ }
Review comment:
I think you should also check the legalness of the path. It should start with "root' but cannot be "root".
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1397,6 +1401,91 @@ public TSStatus executeNonQuery(PhysicalPlan plan) {
}
}
+
+ public TSStatus validatePlan(PhysicalPlan plan) {
+ TSStatus result = StatusUtils.OK;
+ if (plan instanceof SetStorageGroupPlan) {
+ return validateSetStorageGroupPlan((SetStorageGroupPlan) plan);
+ } else if (plan instanceof InsertPlan) {
+ return validateInsertPlan((InsertPlan) plan);
+ }
+ // TODO add more pre-validations
+ return result;
+ }
+
+ private TSStatus validateSetStorageGroupPlan(SetStorageGroupPlan plan) {
+ String path = plan.getPath().getFullPath();
+ if (getAllStorageGroupNames().contains(path)) {
+ return StatusUtils.PATH_ALREADY_EXIST_ERROR;
+ }
+ return StatusUtils.OK;
+ }
+
+ private TSStatus validateInsertPlan(InsertPlan plan) {
+ List<Path> paths = (plan).getPaths();
+ String[] values = (plan).getValues();
+ List<String> pathStrings = new ArrayList<>();
+ for (Path path : paths) {
+ pathStrings.add(path.getFullPath());
+ }
+ List<MeasurementSchema> measurementSchemas;
+ try {
+ measurementSchemas = pullTimeSeriesSchemas(pathStrings);
+ } catch (MetadataException e) {
+ return StatusUtils.NO_STORAGE_GROUP;
+ }
+ for (int i = 0; i < paths.size(); i++) {
+ String measurement = paths.get(i).getMeasurement();
+ String value = values[i];
+ TSStatus result = null;
+ for (MeasurementSchema schema : measurementSchemas) {
+ if (schema.getMeasurementId().equals(measurement)) {
+ try {
+ tryParse(value, schema.getType());
+ result = StatusUtils.OK;
+ } catch (IllegalArgumentException e) {
+ result = new TSStatus(StatusUtils.WRITE_PROCESS_ERROR.getCode());
+ result.setMessage(StatusUtils.WRITE_PROCESS_ERROR.getMessage() + e.getMessage());
+ return result;
+ }
+ break;
+ }
+ }
+ if (result == null) {
+ return StatusUtils.PATH_NOT_EXIST_ERROR;
+ }
+ }
+ return StatusUtils.OK;
+ }
+
+
+ private void tryParse(String value, TSDataType type) {
+ switch (type) {
+ case INT32:
+ Integer.parseInt(value);
+ break;
+ case INT64:
+ Long.parseLong(value);
+ break;
+ case BOOLEAN:
+ if (!(value.equals("true") || value.equals("TRUE")
+ || value.equals("false") || value.equals("FALSE")
+ || value.equals("0") || value.equals("1"))) {
Review comment:
We have defined these strings in SQLConstants, better to use them. And maybe you can use `equalsIgnoreCase` to increase robustness.
----------------------------------------------------------------
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
[GitHub] [incubator-iotdb] jt2594838 merged pull request #1230: [IOTDB-665] validate physical plan before fowarding
Posted by GitBox <gi...@apache.org>.
jt2594838 merged pull request #1230:
URL: https://github.com/apache/incubator-iotdb/pull/1230
----------------------------------------------------------------
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
[GitHub] [incubator-iotdb] Ring-k commented on a change in pull request #1230: [IOTDB-665] validate physical plan before fowarding
Posted by GitBox <gi...@apache.org>.
Ring-k commented on a change in pull request #1230:
URL: https://github.com/apache/incubator-iotdb/pull/1230#discussion_r429964808
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1397,6 +1401,91 @@ public TSStatus executeNonQuery(PhysicalPlan plan) {
}
}
+
+ public TSStatus validatePlan(PhysicalPlan plan) {
+ TSStatus result = StatusUtils.OK;
+ if (plan instanceof SetStorageGroupPlan) {
+ return validateSetStorageGroupPlan((SetStorageGroupPlan) plan);
+ } else if (plan instanceof InsertPlan) {
+ return validateInsertPlan((InsertPlan) plan);
+ }
+ // TODO add more pre-validations
+ return result;
+ }
+
+ private TSStatus validateSetStorageGroupPlan(SetStorageGroupPlan plan) {
+ String path = plan.getPath().getFullPath();
+ if (getAllStorageGroupNames().contains(path)) {
+ return StatusUtils.PATH_ALREADY_EXIST_ERROR;
+ }
Review comment:
That's right. Thanks for your reminding.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1397,6 +1401,91 @@ public TSStatus executeNonQuery(PhysicalPlan plan) {
}
}
+
+ public TSStatus validatePlan(PhysicalPlan plan) {
+ TSStatus result = StatusUtils.OK;
+ if (plan instanceof SetStorageGroupPlan) {
+ return validateSetStorageGroupPlan((SetStorageGroupPlan) plan);
+ } else if (plan instanceof InsertPlan) {
+ return validateInsertPlan((InsertPlan) plan);
+ }
+ // TODO add more pre-validations
+ return result;
+ }
+
+ private TSStatus validateSetStorageGroupPlan(SetStorageGroupPlan plan) {
+ String path = plan.getPath().getFullPath();
+ if (getAllStorageGroupNames().contains(path)) {
+ return StatusUtils.PATH_ALREADY_EXIST_ERROR;
+ }
+ return StatusUtils.OK;
+ }
+
+ private TSStatus validateInsertPlan(InsertPlan plan) {
+ List<Path> paths = (plan).getPaths();
+ String[] values = (plan).getValues();
+ List<String> pathStrings = new ArrayList<>();
+ for (Path path : paths) {
+ pathStrings.add(path.getFullPath());
+ }
+ List<MeasurementSchema> measurementSchemas;
+ try {
+ measurementSchemas = pullTimeSeriesSchemas(pathStrings);
Review comment:
The schema cache can help to accelerate the validations. Thanks for your suggestions.
##########
File path: cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
##########
@@ -1397,6 +1401,91 @@ public TSStatus executeNonQuery(PhysicalPlan plan) {
}
}
+
+ public TSStatus validatePlan(PhysicalPlan plan) {
+ TSStatus result = StatusUtils.OK;
+ if (plan instanceof SetStorageGroupPlan) {
+ return validateSetStorageGroupPlan((SetStorageGroupPlan) plan);
+ } else if (plan instanceof InsertPlan) {
+ return validateInsertPlan((InsertPlan) plan);
+ }
+ // TODO add more pre-validations
+ return result;
+ }
+
+ private TSStatus validateSetStorageGroupPlan(SetStorageGroupPlan plan) {
+ String path = plan.getPath().getFullPath();
+ if (getAllStorageGroupNames().contains(path)) {
+ return StatusUtils.PATH_ALREADY_EXIST_ERROR;
+ }
+ return StatusUtils.OK;
+ }
+
+ private TSStatus validateInsertPlan(InsertPlan plan) {
+ List<Path> paths = (plan).getPaths();
+ String[] values = (plan).getValues();
+ List<String> pathStrings = new ArrayList<>();
+ for (Path path : paths) {
+ pathStrings.add(path.getFullPath());
+ }
+ List<MeasurementSchema> measurementSchemas;
+ try {
+ measurementSchemas = pullTimeSeriesSchemas(pathStrings);
+ } catch (MetadataException e) {
+ return StatusUtils.NO_STORAGE_GROUP;
+ }
+ for (int i = 0; i < paths.size(); i++) {
+ String measurement = paths.get(i).getMeasurement();
+ String value = values[i];
+ TSStatus result = null;
+ for (MeasurementSchema schema : measurementSchemas) {
+ if (schema.getMeasurementId().equals(measurement)) {
+ try {
+ tryParse(value, schema.getType());
+ result = StatusUtils.OK;
+ } catch (IllegalArgumentException e) {
+ result = new TSStatus(StatusUtils.WRITE_PROCESS_ERROR.getCode());
+ result.setMessage(StatusUtils.WRITE_PROCESS_ERROR.getMessage() + e.getMessage());
+ return result;
+ }
+ break;
+ }
+ }
+ if (result == null) {
+ return StatusUtils.PATH_NOT_EXIST_ERROR;
+ }
+ }
+ return StatusUtils.OK;
+ }
+
+
+ private void tryParse(String value, TSDataType type) {
+ switch (type) {
+ case INT32:
+ Integer.parseInt(value);
+ break;
+ case INT64:
+ Long.parseLong(value);
+ break;
+ case BOOLEAN:
+ if (!(value.equals("true") || value.equals("TRUE")
+ || value.equals("false") || value.equals("FALSE")
+ || value.equals("0") || value.equals("1"))) {
Review comment:
Oh, I didn't notice that. That's convenient. Thanks for your suggestion :)
----------------------------------------------------------------
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