[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #8160: Fix problem about delete multi table , select into and select lock statement.

[GitBox <gi...@apache.org>]

strongduanmu commented on a change in pull request #8160:
URL: https://github.com/apache/shardingsphere/pull/8160#discussion_r524882318



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/antlr4/imports/mysql/DMLStatement.g4
##########
@@ -108,10 +108,6 @@ multipleTablesClause
     : tableAliasRefList FROM tableReferences | FROM tableAliasRefList USING tableReferences
     ;
 
-multipleTableNames

Review comment:
       Why delete `multipleTableNames ` rule? Useless?

##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/impl/MySQLStatementSQLVisitor.java
##########
@@ -544,7 +545,17 @@ public ASTNode visitQueryExpressionParens(final QueryExpressionParensContext ctx
 
     @Override
     public ASTNode visitLockClauseList(final LockClauseListContext ctx) {
-        return new LockSegment(ctx.getStart().getStartIndex(), ctx.getStop().getStopIndex());
+        LockSegment lockSegment = new LockSegment(ctx.getStart().getStartIndex(), ctx.getStop().getStopIndex());

Review comment:
       Please use the `result` for the return variable name.
   

##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/segment/dml/predicate/LockSegment.java
##########
@@ -32,4 +36,7 @@
     private final int startIndex;
 
     private final int stopIndex;
+
+    @Setter
+    private List<SimpleTableSegment> forTables;

Review comment:
       Maybe it is better to initialize the collection in advance? Like this:
   
   ```java
   private final List< SimpleTableSegment > tables = new LinkedList<>();
   ```
   What do you think? 😀

##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/impl/MySQLStatementSQLVisitor.java
##########
@@ -544,7 +545,17 @@ public ASTNode visitQueryExpressionParens(final QueryExpressionParensContext ctx
 
     @Override
     public ASTNode visitLockClauseList(final LockClauseListContext ctx) {
-        return new LockSegment(ctx.getStart().getStartIndex(), ctx.getStop().getStopIndex());
+        LockSegment lockSegment = new LockSegment(ctx.getStart().getStartIndex(), ctx.getStop().getStopIndex());
+        List<SimpleTableSegment> forTables = new LinkedList<>();
+        for (MySQLStatementParser.LockClauseContext each : ctx.lockClause()) {
+            if (null != each.tableLockingList()) {
+                forTables.addAll(generateTablesFromTableAliasRefList(each.tableLockingList().tableAliasRefList()));

Review comment:
       The intermediate variable `forTables` is a bit redundant, how about this? 
   
   ```java
   lockSegment.getForTables.addAll(generateTablesFromTableAliasRefList(each.tableLockingList().tableAliasRefList()));
   ```
   




----------------------------------------------------------------
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