github-actions[bot] commented on PR #1140: URL: https://github.com/apache/incubator-seata-go/pull/1140#issuecomment-5301187902
> Bot detected the issue body's language is not English, translate it automatically. > Thank you for the review, the following points are the design intentions pointed out by the corresponding review > > 1. In fact, the aggregation path returns the last result to the user, because the underlying driver uses` go-sql-driver/mysql v1.6.0 `, which must be upgraded to 1.8.0 to get a sum RowsAffected in the case of an aggregation.If you want to upgrade, feel that you can do it in the follow-up pr.So here are the last results returned by both paths, I will add a comment > 2. I haven't thought of a better plan to let the sql registered with `update/delete hook` take the aggregation path, I first let him take the sequence and then supplement the log > 3. This` HooksForSQLType `is not only used to judge the length, the sequential subexecutor will also call > > `` `go > childHooks: = hooksForSQLType (parseCtx.SQLType)//multi_sequential_executor.go: 148 > ``` >> In addition, in the aggregation judgment, only those with hooks will be copied, and only the first statement with hooks will be copied, and only one slice copy will be wasted. > > 4. `rejectATPreparedMultiSQL' fails open Here is to reject only multi-SQL that can be confirmed, and the execution phase will still be parsed normally.(I'll add a comment) > 5. `execSequential` Reparse each statement is not just to check if the actuator type has changed, this is mainly to reset the index of the sub-statement parameter placeholder to prevent out-of-bounds/incorrect parameters: > > When multi-sentence global parsing, the placeholder sequence number is globally continuous, for example: > > ``` > update t_user set name =? Where id =?; > delete from t_user where user_id =?; > ``` > > The sequence number of this parameter in the overall AST is: `Update: 0,1` `delete: 2` But when executed sequentially, the code is to split the parameter into its own slice of the sub-statement and reset the `driver.NamedValue.Ordinal`: >> ``` > Update args: [name, id] > delete args: [user_id] > ``` > > This` TestReparseStatementResetsParameterOrders (214) `test in` multi_sequential_executor_test.go `has been validated. I'll add a comment next. > > 6. I intend to implement this in the follow-up pr > 7. Point 8 is unintentional, I will adjust Okay, thank you very much for the answer. Because the code is a bit long, I haven't finished crit yet, but so far it seems there aren't any major problems. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
