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/17 04:04:40 UTC
[GitHub] [shardingsphere] jingshanglu opened a new pull request #6370: fix sql test
jingshanglu opened a new pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370
Changes proposed in this pull request:
- fix sql test case select_with_expression
-fix sql test case select_pagination_with_offset_fetch
----------------------------------------------------------------
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 #6370: fix sql test
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370#discussion_r456775949
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java
##########
@@ -470,7 +470,8 @@ private ASTNode createProjection(final ProjectionContext ctx, final AliasSegment
}
if (projection instanceof CommonExpressionSegment) {
CommonExpressionSegment segment = (CommonExpressionSegment) projection;
- ExpressionProjectionSegment result = new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText());
+ ExpressionProjectionSegment result = null == alias ? new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText())
+ : new ExpressionProjectionSegment(segment.getStartIndex(), alias.getStopIndex(), segment.getText());
Review comment:
Do you think it is better to overwrite `getStopIndex()` of `ExpressionProjectionSegment ` with the stopIndex of its`aliasSegment`,
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #6370: fix sql test
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370#discussion_r458477199
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java
##########
@@ -470,7 +470,8 @@ private ASTNode createProjection(final ProjectionContext ctx, final AliasSegment
}
if (projection instanceof CommonExpressionSegment) {
CommonExpressionSegment segment = (CommonExpressionSegment) projection;
- ExpressionProjectionSegment result = new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText());
+ ExpressionProjectionSegment result = null == alias ? new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText())
+ : new ExpressionProjectionSegment(segment.getStartIndex(), alias.getStopIndex(), segment.getText());
Review comment:
> Do you think it is better to overwrite `getStopIndex()` of `ExpressionProjectionSegment ` with the stopIndex of its`aliasSegment`,
Ok
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #6370: fix sql test
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370#discussion_r458489794
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java
##########
@@ -470,7 +470,8 @@ private ASTNode createProjection(final ProjectionContext ctx, final AliasSegment
}
if (projection instanceof CommonExpressionSegment) {
CommonExpressionSegment segment = (CommonExpressionSegment) projection;
- ExpressionProjectionSegment result = new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText());
+ ExpressionProjectionSegment result = null == alias ? new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText())
+ : new ExpressionProjectionSegment(segment.getStartIndex(), alias.getStopIndex(), segment.getText());
Review comment:
> > Do you think it is better to overwrite `getStopIndex()` of `ExpressionProjectionSegment ` with the stopIndex of its`aliasSegment`,
>
> I don't think so because expressionProjectionSegment contains alias.
----------------------------------------------------------------
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 #6370: fix sql test
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370
----------------------------------------------------------------
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 #6370: fix sql test
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370#discussion_r458654623
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-postgresql/src/main/java/org/apache/shardingsphere/sql/parser/postgresql/visitor/PostgreSQLVisitor.java
##########
@@ -262,14 +262,15 @@ private PredicateSegment createInSegment(final AExprContext ctx) {
private Collection<ExpressionSegment> getExpressionSegments(final AExprContext ctx) {
Collection<ExpressionSegment> result = new LinkedList<>();
-// if (null != ctx.cExpr() && null != ctx.cExpr().select_with_parens()) {
-// Select_with_parensContext subquery = ctx.cExpr().select_with_parens();
+// TODO deal with sqlExpressions in PredicateInRightValue
+// if (null != ctx.cExpr() && null != ctx.cExpr().selectWithParens()) {
+// PostgreSQLStatementParser.SelectWithParensContext subquery = ctx.cExpr().selectWithParens();
// result.add(new SubqueryExpressionSegment(new SubquerySegment(subquery.getStart().getStartIndex(), subquery.getStop().getStopIndex(),
-// (SelectStatement) visit(ctx.cExpr().select_with_parens()))));
Review comment:
Do you think these codes can be removed?
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #6370: fix sql test
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370#discussion_r458489794
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java
##########
@@ -470,7 +470,8 @@ private ASTNode createProjection(final ProjectionContext ctx, final AliasSegment
}
if (projection instanceof CommonExpressionSegment) {
CommonExpressionSegment segment = (CommonExpressionSegment) projection;
- ExpressionProjectionSegment result = new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText());
+ ExpressionProjectionSegment result = null == alias ? new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText())
+ : new ExpressionProjectionSegment(segment.getStartIndex(), alias.getStopIndex(), segment.getText());
Review comment:
> > Do you think it is better to overwrite `getStopIndex()` of `ExpressionProjectionSegment ` with the stopIndex of its`aliasSegment`,
>
> I don't think so because expressionProjectionSegment contains alias.
----------------------------------------------------------------
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] jingshanglu commented on a change in pull request #6370: fix sql test
Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #6370:
URL: https://github.com/apache/shardingsphere/pull/6370#discussion_r458477199
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java
##########
@@ -470,7 +470,8 @@ private ASTNode createProjection(final ProjectionContext ctx, final AliasSegment
}
if (projection instanceof CommonExpressionSegment) {
CommonExpressionSegment segment = (CommonExpressionSegment) projection;
- ExpressionProjectionSegment result = new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText());
+ ExpressionProjectionSegment result = null == alias ? new ExpressionProjectionSegment(segment.getStartIndex(), segment.getStopIndex(), segment.getText())
+ : new ExpressionProjectionSegment(segment.getStartIndex(), alias.getStopIndex(), segment.getText());
Review comment:
> Do you think it is better to overwrite `getStopIndex()` of `ExpressionProjectionSegment ` with the stopIndex of its`aliasSegment`,
Ok
----------------------------------------------------------------
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