JackieTien97 commented on code in PR #15685:
URL: https://github.com/apache/iotdb/pull/15685#discussion_r2176227149
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/client/DataNodeInternalClient.java:
##########
@@ -98,6 +111,33 @@ public TSStatus insertTablets(InsertMultiTabletsStatement
statement) {
}
}
+ public TSStatus insertRelationalTablets(InsertMultiTabletsStatement
statement) {
+ try {
+ // permission check
+ TSStatus status = AuthorityChecker.checkAuthority(statement, session);
+ if (status.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return status;
+ }
+ // call the coordinator
+ long queryId = SESSION_MANAGER.requestQueryId();
+ SessionManager.getInstance().registerSession(session);
Review Comment:
```suggestion
```
we should never do this, because one thread may serve different insert into
query tasks concurrently
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/client/DataNodeInternalClient.java:
##########
@@ -98,6 +111,33 @@ public TSStatus insertTablets(InsertMultiTabletsStatement
statement) {
}
}
+ public TSStatus insertRelationalTablets(InsertMultiTabletsStatement
statement) {
Review Comment:
for table model, there is no need to use `InsertMultiTabletsStatement`, it
should only be `InsertTabletStatement`
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/rest/table/v1/impl/RestApiServiceImpl.java:
##########
@@ -91,7 +99,7 @@ public Response executeQueryStatement(SQL sql,
SecurityContext securityContext)
.build();
}
- if (ExecuteStatementHandler.validateStatement(statement)) {
+ if (!skipValidateStatement &&
ExecuteStatementHandler.validateStatement(statement)) {
Review Comment:
in which case we need to skip the validateStatement? It seems that even if
insert into query, it's Insert Statement? and is allowed in `validateStatement`
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/statement/crud/InsertBaseStatement.java:
##########
@@ -185,6 +185,11 @@ public TSStatus checkPermissionBeforeProcess(String
userName) {
if (AuthorityChecker.SUPER_USER.equals(userName)) {
return new TSStatus(TSStatusCode.SUCCESS_STATUS.getStatusCode());
}
+ // WRITE_DATA permission should not be required by table model insert
+ if (isWriteToTable()) {
+ return new TSStatus(TSStatusCode.SUCCESS_STATUS.getStatusCode());
+ }
+
Review Comment:
```suggestion
```
should never be called for table model
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/schema/column/ColumnHeaderConstant.java:
##########
@@ -567,6 +568,9 @@ private ColumnHeaderConstant() {
new ColumnHeader(TARGET_TIMESERIES, TSDataType.TEXT),
new ColumnHeader(WRITTEN, TSDataType.INT32));
+ public static final List<ColumnHeader> selectIntoTableColumnHeaders =
+ ImmutableList.of(new ColumnHeader(ROWS, TSDataType.INT32));
Review Comment:
better use INT64
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/rest/table/v1/impl/RestApiServiceImpl.java:
##########
@@ -125,10 +133,15 @@ public Response executeQueryStatement(SQL sql,
SecurityContext securityContext)
}
IQueryExecution queryExecution = COORDINATOR.getQueryExecution(queryId);
try (SetThreadName threadName = new
SetThreadName(result.queryId.getId())) {
- return QueryDataSetHandler.fillQueryDataSet(
- queryExecution,
- statement,
- sql.getRowLimit() == null ? defaultQueryRowLimit :
sql.getRowLimit());
+ Response res =
+ QueryDataSetHandler.fillQueryDataSet(
+ queryExecution,
+ statement,
+ sql.getRowLimit() == null ? defaultQueryRowLimit :
sql.getRowLimit());
+ if (queryExecution.getQueryType() == QueryType.READ_WRITE) {
+ return responseGenerateHelper(result);
+ }
+ return res;
Review Comment:
```suggestion
if (queryExecution.getQueryType() == QueryType.READ_WRITE) {
return responseGenerateHelper(result);
} else {
return QueryDataSetHandler.fillQueryDataSet(
queryExecution,
statement,
sql.getRowLimit() == null ? defaultQueryRowLimit :
sql.getRowLimit());
}
```
No need to construct `Response` at first.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/client/DataNodeInternalClient.java:
##########
@@ -98,6 +111,33 @@ public TSStatus insertTablets(InsertMultiTabletsStatement
statement) {
}
}
+ public TSStatus insertRelationalTablets(InsertMultiTabletsStatement
statement) {
Review Comment:
One Tablet can contain many devices' data for table model
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/client/DataNodeInternalClient.java:
##########
@@ -98,6 +111,33 @@ public TSStatus insertTablets(InsertMultiTabletsStatement
statement) {
}
}
+ public TSStatus insertRelationalTablets(InsertMultiTabletsStatement
statement) {
+ try {
+ // permission check
+ TSStatus status = AuthorityChecker.checkAuthority(statement, session);
+ if (status.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return status;
+ }
Review Comment:
permission check shouldn't happen here for table model, it happens in
analyze stage, `AuthorityChecker.checkAuthority` is only used for tree model.
You can refer to some comments in `insertTablet` method of
`ClientRPCServiceImpl`
many steps are only for tree model
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/rest/table/v1/impl/RestApiServiceImpl.java:
##########
@@ -224,6 +237,11 @@ public Response executeNonQueryStatement(SQL sql,
SecurityContext securityContex
.message(TSStatusCode.EXECUTE_STATEMENT_ERROR.name()))
.build();
}
+
+ if (statement instanceof Insert) {
Review Comment:
It seems that you've already done
`ExecuteStatementHandler.validateStatement(statement)` in previous step, is
there any need to call `executeQueryStatement` and set skipValidate parameter
to true?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/rest/table/v1/impl/RestApiServiceImpl.java:
##########
@@ -125,10 +133,15 @@ public Response executeQueryStatement(SQL sql,
SecurityContext securityContext)
}
IQueryExecution queryExecution = COORDINATOR.getQueryExecution(queryId);
try (SetThreadName threadName = new
SetThreadName(result.queryId.getId())) {
- return QueryDataSetHandler.fillQueryDataSet(
- queryExecution,
- statement,
- sql.getRowLimit() == null ? defaultQueryRowLimit :
sql.getRowLimit());
+ Response res =
+ QueryDataSetHandler.fillQueryDataSet(
+ queryExecution,
+ statement,
+ sql.getRowLimit() == null ? defaultQueryRowLimit :
sql.getRowLimit());
+ if (queryExecution.getQueryType() == QueryType.READ_WRITE) {
+ return responseGenerateHelper(result);
+ }
+ return res;
Review Comment:
in the finally block, the `OperationType` will not always be
`EXECUTE_QUERY_STATEMENT`
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/client/DataNodeInternalClient.java:
##########
@@ -98,6 +111,33 @@ public TSStatus insertTablets(InsertMultiTabletsStatement
statement) {
}
}
+ public TSStatus insertRelationalTablets(InsertMultiTabletsStatement
statement) {
+ try {
+ // permission check
+ TSStatus status = AuthorityChecker.checkAuthority(statement, session);
+ if (status.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) {
+ return status;
+ }
+ // call the coordinator
+ long queryId = SESSION_MANAGER.requestQueryId();
+ SessionManager.getInstance().registerSession(session);
+ ExecutionResult result =
+ COORDINATOR.executeForTableModel(
+ statement.getInsertTabletStatementList().get(0),
+ relationSqlParser,
+ session,
+ queryId,
+ SESSION_MANAGER.getSessionInfo(session),
+ "",
+ metadata,
+ config.getConnectionTimeoutInMS());
+ return result.status;
+ } catch (final Exception e) {
+ return onQueryException(
+ e, OperationType.INSERT_TABLETS.getName(),
TSStatusCode.EXECUTE_STATEMENT_ERROR);
+ }
Review Comment:
```suggestion
} catch (IoTDBException e) {
return onIoTDBException(e, OperationType.SELECT_INTO,
e.getErrorCode());
} catch (Exception e) {
return onQueryException(
e, OperationType.SELECT_INTO.getName(),
TSStatusCode.EXECUTE_STATEMENT_ERROR);
} finally {
CommonUtils.addStatementExecutionLatency(
OperationType.SELECT_INTO, StatementType.SELECT_INTO.name(),
System.nanoTime() - t1);
}
```
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/execution/operator/MergeTreeSortOperatorTest.java:
##########
@@ -1900,6 +1901,11 @@ public boolean isQuery() {
return false;
}
+ @Override
+ public QueryType getQueryType() {
+ return null;
Review Comment:
READ?
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/protocol/thrift/impl/ClientRPCServiceImpl.java:
##########
@@ -432,6 +433,10 @@ private TSExecuteStatementResp executeStatementInternal(
if (quota != null) {
quota.addReadResult(resp.getQueryResult());
}
+ // Should return SUCCESS_MESSAGE for insert into query
+ if (queryExecution.getQueryType() == QueryType.READ_WRITE) {
+ resp.setColumns(null);
+ }
Review Comment:
should be same as the else block?
```
finished = true;
resp = RpcUtils.getTSExecuteStatementResp(result.status);
```
and should put these two lines at the very begining of the if block
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
