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/25 10:26:45 UTC
[GitHub] [incubator-iotdb] jt2594838 commented on a change in pull request #1230: [IOTDB-665] validate physical plan before fowarding
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