You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2020/07/19 15:25:01 UTC
[GitHub] [shardingsphere] strongduanmu opened a new pull request #6380: parse innerExpression and remove sql parameter in SelectStatementContext
strongduanmu opened a new pull request #6380:
URL: https://github.com/apache/shardingsphere/pull/6380
Ref #6289.
Changes proposed in this pull request:
- parse `innerExpression` in parse phrase to replace `innerExpressionStartIndex`;
- remove sql parameter in SelectStatementContext;
----------------------------------------------------------------
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
[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #6380: parse innerExpression and remove sql parameter in SelectStatementContext
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6380:
URL: https://github.com/apache/shardingsphere/pull/6380#discussion_r457181138
##########
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?
@tristaZero According to the documentation of the `ctx.getText()` method, we can find that the method does not return whitespace.
```
/**
* Return the combined text of all leaf nodes. Does not get any
* off-channel tokens (if any) so won't return whitespace and
* comments if they are sent to parser on hidden channel.
*/
String getText();
```
In order to get the original sql that contains whitespace, we need to use the `ctx.start.getInputStream().getText()` method to specify the start and end interval through Interval.
----------------------------------------------------------------
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
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #6380: parse innerExpression and remove sql parameter in SelectStatementContext
Posted by 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
[GitHub] [shardingsphere] tristaZero merged pull request #6380: parse innerExpression and remove sql parameter in SelectStatementContext
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #6380:
URL: https://github.com/apache/shardingsphere/pull/6380
----------------------------------------------------------------
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
[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #6380: parse innerExpression and remove sql parameter in SelectStatementContext
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6380:
URL: https://github.com/apache/shardingsphere/pull/6380#discussion_r457209681
##########
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()?
Yes, maybe using `ctx.LP_().getSymbol().getStartIndex()` is more easier to read. I will optimise it.
> From the definition of aggregationFunction ,
>
> aggregationFunction
> : aggregationFunctionName LP_ distinct? (expr (COMMA_ expr)* | ASTERISK_)? RP_
> What we want is from LP_ to RP_, right?
Yes, the innerExpression include sql string from LP_ to RP_. The previous innerExpressionStartIndex is also the same logic.
----------------------------------------------------------------
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
[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #6380: parse innerExpression and remove sql parameter in SelectStatementContext
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6380:
URL: https://github.com/apache/shardingsphere/pull/6380#discussion_r457209889
##########
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:
I will fix it.
----------------------------------------------------------------
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