[GitHub] [shardingsphere] tristaZero commented on a change in pull request #6380: parse innerExpression and remove sql parameter in SelectStatementContext

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

tristaZero commented on a change in pull request #6380:
URL: https://github.com/apache/shardingsphere/pull/6380#discussion_r457094184



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/java/org/apache/shardingsphere/sql/parser/sqlserver/visitor/SQLServerVisitor.java
##########
@@ -430,11 +431,11 @@ public final ASTNode visitAggregationFunction(final AggregationFunctionContext c
     
     private ASTNode createAggregationSegment(final AggregationFunctionContext ctx, final String aggregationType) {
         AggregationType type = AggregationType.valueOf(aggregationType.toUpperCase());
-        int innerExpressionStartIndex = ((TerminalNode) ctx.getChild(1)).getSymbol().getStartIndex();
+        String innerExpression = ctx.start.getInputStream().getText(new Interval(((TerminalNode) ctx.getChild(1)).getSymbol().getStartIndex(), ctx.stop.getStopIndex()));

Review comment:
       The result of `ctx.start.getInputStream().getText()` and `ctx.getText()` are the same?

##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/java/org/apache/shardingsphere/sql/parser/sqlserver/visitor/SQLServerVisitor.java
##########
@@ -430,11 +431,11 @@ public final ASTNode visitAggregationFunction(final AggregationFunctionContext c
     
     private ASTNode createAggregationSegment(final AggregationFunctionContext ctx, final String aggregationType) {
         AggregationType type = AggregationType.valueOf(aggregationType.toUpperCase());
-        int innerExpressionStartIndex = ((TerminalNode) ctx.getChild(1)).getSymbol().getStartIndex();
+        String innerExpression = ctx.start.getInputStream().getText(new Interval(((TerminalNode) ctx.getChild(1)).getSymbol().getStartIndex(), ctx.stop.getStopIndex()));

Review comment:
        Do you think `ctx.getChild(1)).getSymbol().getStartIndex()` == `ctx.LP_().getSymbol().getStartIndex()`?
   
   From the definition of `aggregationFunction `,
   ```xml
   aggregationFunction
       : aggregationFunctionName LP_ distinct? (expr (COMMA_ expr)* | ASTERISK_)? RP_
   ```
   What we want is from LP_ to RP_, right?

##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/MySQLVisitor.java
##########
@@ -339,11 +340,11 @@ private PredicateSegment createInSegment(final PredicateContext ctx) {
     }
     
     private PredicateBracketValue createBracketValue(final PredicateContext ctx) {
-        PredicateLeftBracketValue predicateLeftBracketValue = null != ctx.subquery() 
-                ? new PredicateLeftBracketValue(ctx.subquery().LP_().getSymbol().getStartIndex(), ctx.subquery().LP_().getSymbol().getStopIndex()) 
+        PredicateLeftBracketValue predicateLeftBracketValue = null != ctx.subquery()
+                ? new PredicateLeftBracketValue(ctx.subquery().LP_().getSymbol().getStartIndex(), ctx.subquery().LP_().getSymbol().getStopIndex())
                 : new PredicateLeftBracketValue(ctx.LP_().getSymbol().getStartIndex(), ctx.LP_().getSymbol().getStopIndex());
-        PredicateRightBracketValue predicateRightBracketValue = null != ctx.subquery() 

Review comment:
       Here the check style is out of rule, so you correct them?




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