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/08/22 14:33:22 UTC
[GitHub] [shardingsphere] strongduanmu opened a new pull request #6997: fix update/delete limit parameterCount parse error
strongduanmu opened a new pull request #6997:
URL: https://github.com/apache/shardingsphere/pull/6997
Fixes #6939 .
Changes proposed in this pull request:
- fix update/delete limit parameterCount parse error
- add check when update/delete limit statement route to multiple data nodes
----------------------------------------------------------------
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 #6997: fix update/delete limit parameterCount parse error
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #6997:
URL: https://github.com/apache/shardingsphere/pull/6997
----------------------------------------------------------------
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] coveralls commented on pull request #6997: fix update/delete limit parameterCount parse error
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #6997:
URL: https://github.com/apache/shardingsphere/pull/6997#issuecomment-678651488
## Pull Request Test Coverage Report for [Build 14027](https://coveralls.io/builds/32931209)
* **14** of **19** **(73.68%)** changed or added relevant lines in **5** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.02%**) to **35.058%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/ShardingRouteDecorator.java](https://coveralls.io/builds/32931209/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-route%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Froute%2Fengine%2FShardingRouteDecorator.java#L77) | 4 | 5 | 80.0%
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/DeleteStatement.java](https://coveralls.io/builds/32931209/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fstatement%2Fdml%2FDeleteStatement.java#L61) | 0 | 2 | 0.0%
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/UpdateStatement.java](https://coveralls.io/builds/32931209/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fstatement%2Fdml%2FUpdateStatement.java#L64) | 0 | 2 | 0.0%
<!-- | **Total:** | **14** | **19** | **73.68%** | -->
| Totals | [](https://coveralls.io/builds/32931209) |
| :-- | --: |
| Change from base [Build 14026](https://coveralls.io/builds/32929458): | 0.02% |
| Covered Lines: | 35220 |
| Relevant Lines: | 100461 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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 #6997: fix update/delete limit parameterCount parse error
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6997:
URL: https://github.com/apache/shardingsphere/pull/6997#discussion_r475168749
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/ShardingRouteDecorator.java
##########
@@ -70,9 +73,18 @@ public RouteContext decorate(final RouteContext routeContext, final ShardingSphe
}
ShardingRouteEngine shardingRouteEngine = ShardingRouteEngineFactory.newInstance(shardingRule, metaData, sqlStatementContext, shardingConditions, props);
RouteResult routeResult = shardingRouteEngine.route(shardingRule);
+ if (containsUpdateDeletePagination(sqlStatement) && routeResult.getRouteUnits().size() > 1) {
Review comment:
> IMO, this condition judgment for `update and delete statements` seems a little bit intrusive for `sharding router`. I mean it is a `detailed statement judgment` for `router` seemingly. Do you think it is possible to move this to the `statement validator`.
>
> However, the current `statement validator` maybe not meet our needs now. Hence other changes for it are required. My feeling is that we can rename the original `validate` function of `validator` as `preValidate` (A draft name, welcome a better one) firstly. Moreover, this added validating condition could be viewed as a `postValidate` function for `validator`.
>
> I'd like to listen to your voice.
> Bets,
> Trista
@tristaZero I really like the design of `preValidate` and `postValidate`, which will make the logic clearer.
----------------------------------------------------------------
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] coveralls edited a comment on pull request #6997: fix update/delete limit parameterCount parse error
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #6997:
URL: https://github.com/apache/shardingsphere/pull/6997#issuecomment-678651488
## Pull Request Test Coverage Report for [Build 14031](https://coveralls.io/builds/32935728)
* **14** of **24** **(58.33%)** changed or added relevant lines in **7** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.06%**) to **35.103%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/DeleteStatement.java](https://coveralls.io/builds/32935728/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fstatement%2Fdml%2FDeleteStatement.java#L61) | 0 | 2 | 0.0%
| [shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/UpdateStatement.java](https://coveralls.io/builds/32935728/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-statement%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Fsql%2Fstatement%2Fdml%2FUpdateStatement.java#L64) | 0 | 2 | 0.0%
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/impl/ShardingDeleteStatementValidator.java](https://coveralls.io/builds/32935728/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-route%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Froute%2Fengine%2Fvalidator%2Fimpl%2FShardingDeleteStatementValidator.java#L45) | 0 | 3 | 0.0%
| [shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/impl/ShardingUpdateStatementValidator.java](https://coveralls.io/builds/32935728/source?filename=shardingsphere-features%2Fshardingsphere-sharding%2Fshardingsphere-sharding-route%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsharding%2Froute%2Fengine%2Fvalidator%2Fimpl%2FShardingUpdateStatementValidator.java#L74) | 0 | 3 | 0.0%
<!-- | **Total:** | **14** | **24** | **58.33%** | -->
| Totals | [](https://coveralls.io/builds/32935728) |
| :-- | --: |
| Change from base [Build 14026](https://coveralls.io/builds/32929458): | 0.06% |
| Covered Lines: | 35266 |
| Relevant Lines: | 100465 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
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 #6997: fix update/delete limit parameterCount parse error
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #6997:
URL: https://github.com/apache/shardingsphere/pull/6997#discussion_r475161148
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/ShardingRouteDecorator.java
##########
@@ -70,9 +73,18 @@ public RouteContext decorate(final RouteContext routeContext, final ShardingSphe
}
ShardingRouteEngine shardingRouteEngine = ShardingRouteEngineFactory.newInstance(shardingRule, metaData, sqlStatementContext, shardingConditions, props);
RouteResult routeResult = shardingRouteEngine.route(shardingRule);
+ if (containsUpdateDeletePagination(sqlStatement) && routeResult.getRouteUnits().size() > 1) {
Review comment:
IMO, this condition judgment for `update and delete statements` seems a little bit intrusive for `sharding router`. I mean it is a `detailed statement judgment` for `router` seemingly. Do you think it is possible to move this to the `statement validator`.
However, the current `statement validator` maybe not meet our needs now. Hence other changes for it are required. My feeling is that we can rename the original `validate` function of `validator` as `preValidate` (A draft name, welcome a better one) firstly. Moreover, this added validating condition could be viewed as a `postValidate` function for `validator`.
I'd like to listen to your voice.
Bets,
Trista
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/impl/ShardingInsertStatementValidator.java
##########
@@ -48,12 +48,12 @@ public void validate(final ShardingRule shardingRule, final SQLStatementContext<
Optional<OnDuplicateKeyColumnsSegment> onDuplicateKeyColumnsSegment = sqlStatement.getOnDuplicateKeyColumns();
String tableName = sqlStatement.getTable().getTableName().getIdentifier().getValue();
if (onDuplicateKeyColumnsSegment.isPresent() && isUpdateShardingKey(shardingRule, onDuplicateKeyColumnsSegment.get(), tableName)) {
- throw new ShardingSphereException("INSERT INTO .... ON DUPLICATE KEY UPDATE can not support update for sharding column.");
+ throw new ShardingSphereException("INSERT INTO ... ON DUPLICATE KEY UPDATE can not support update for sharding column.");
Review comment:
Polishing work, great. :-)
----------------------------------------------------------------
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