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/18 09:48:02 UTC
[GitHub] [shardingsphere] strongduanmu opened a new pull request #6377: support mysql insert select statement route and rewrite
strongduanmu opened a new pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377
Ref #6289 .
Changes proposed in this pull request:
- support mysql insert select statement route and rewrite
- add some rewrite test cases
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456856372
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/context/ShardingSQLRewriteContextDecorator.java
##########
@@ -38,8 +42,15 @@
@Override
public void decorate(final ShardingRule shardingRule, final ConfigurationProperties props, final SQLRewriteContext sqlRewriteContext, final RouteContext routeContext) {
for (ParameterRewriter each : new ShardingParameterRewriterBuilder(shardingRule, routeContext).getParameterRewriters(sqlRewriteContext.getSchemaMetaData())) {
- if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlRewriteContext.getSqlStatementContext())) {
- each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlRewriteContext.getSqlStatementContext(), sqlRewriteContext.getParameters());
+ SQLStatementContext sqlStatementContext = sqlRewriteContext.getSqlStatementContext();
+ if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlStatementContext)) {
+ each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlStatementContext, sqlRewriteContext.getParameters());
+ }
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()
+ && !sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(((InsertStatementContext) sqlStatementContext).getInsertSelectContext().getSelectStatementContext())) {
+ StandardParameterBuilder parameterBuilder = sqlRewriteContext.getParameterBuilder() instanceof GroupedParameterBuilder
Review comment:
> It is impossible to have many `values(), ()` in `INSERT SELECT`, isn't it?
> If it is that case, there is no chance for `GroupedParameterBuilder` here, right?
> Do you think no estimation for'GroupedParameterBuilder` is reasonable?
There are two reasons for this realization:
1. When initializing SQLRewriteContext, if it is InsertStatementContext, GroupedParameterBuilder will be used to manage parameters.
If the StandardParameterBuilder is used for the insert select statement, it will be necessary to determine whether it is an insert select statement during the rewrite process, and then decide whether to take the value from the StandardParameterBuilder or the GroupedParameterBuilder.
2. Since the select union syntax is not supported now, only one set of select statement parameters will be stored in the GroupedParameterBuilder. After the select union syntax is supported in the future, there will be scenarios containing multiple sets of select statement parameters.
Maybe it's better to add a FIXME here. If we support select union in the future, then modify the rewrite function to support GroupedParameterBuilder parameter.
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456853918
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/context/ShardingSQLRewriteContextDecorator.java
##########
@@ -38,8 +42,15 @@
@Override
public void decorate(final ShardingRule shardingRule, final ConfigurationProperties props, final SQLRewriteContext sqlRewriteContext, final RouteContext routeContext) {
for (ParameterRewriter each : new ShardingParameterRewriterBuilder(shardingRule, routeContext).getParameterRewriters(sqlRewriteContext.getSchemaMetaData())) {
- if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlRewriteContext.getSqlStatementContext())) {
- each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlRewriteContext.getSqlStatementContext(), sqlRewriteContext.getParameters());
+ SQLStatementContext sqlStatementContext = sqlRewriteContext.getSqlStatementContext();
+ if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlStatementContext)) {
+ each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlStatementContext, sqlRewriteContext.getParameters());
+ }
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()
Review comment:
> The content of this condition is a bit long, maybe it is better to exact a function for it.
I will optimize 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
[GitHub] [shardingsphere] strongduanmu commented on a change in pull request #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456920405
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/type/ShardingRouteEngineFactory.java
##########
@@ -94,6 +96,15 @@ public static ShardingRouteEngine newInstance(final ShardingRule shardingRule, f
return getShardingRoutingEngine(shardingRule, sqlStatementContext, shardingConditions, tableNames, props);
}
+ private static Collection<String> getTableNames(final SQLStatementContext sqlStatementContext) {
+ Collection<String> tableNames = new LinkedList<>(sqlStatementContext.getTablesContext().getTableNames());
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()) {
Review comment:
@tristaZero This change was originally for verifying the binding table, maybe revert it is a better choice. The binding table is more suitable for performing verification in ShardingInsertStatementValidator.
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456853167
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/impl/ShardingInsertStatementValidator.java
##########
@@ -35,9 +38,15 @@
@Override
public void validate(final ShardingRule shardingRule, final InsertStatement sqlStatement, final List<Object> parameters) {
Optional<OnDuplicateKeyColumnsSegment> onDuplicateKeyColumnsSegment = sqlStatement.getOnDuplicateKeyColumns();
- if (onDuplicateKeyColumnsSegment.isPresent() && isUpdateShardingKey(shardingRule, onDuplicateKeyColumnsSegment.get(), sqlStatement.getTable().getTableName().getIdentifier().getValue())) {
+ 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.");
}
+ Optional<SubquerySegment> insertSelectSegment = sqlStatement.getInsertSelect();
+ if (insertSelectSegment.isPresent() && isContainsKeyGenerateStrategy(shardingRule, tableName)
+ && !isContainsKeyGenerateColumn(shardingRule, sqlStatement.getColumns(), tableName)) {
+ throw new ShardingSphereException("INSERT INTO .... SELECT can not support key generate column.");
Review comment:
> Do you think it is better to add estimation of `the table inserted and the table selected are the same or bind tables.`?
Adding this check is necessary to reduce the occurrence of abnormal situations.
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456922188
##########
File path: shardingsphere-infra/shardingsphere-infra-rewrite/shardingsphere-infra-rewrite-engine/src/main/java/org/apache/shardingsphere/infra/rewrite/context/SQLRewriteContext.java
##########
@@ -80,5 +80,8 @@ public void addSQLTokenGenerators(final Collection<SQLTokenGenerator> sqlTokenGe
*/
public void generateSQLTokens() {
sqlTokens.addAll(sqlTokenGenerators.generateSQLTokens(sqlStatementContext, parameters, schemaMetaData));
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()) {
Review comment:
@tristaZero Modifying the `isGenerateSQLToken` and `generateSQLTokens` methods in each `tokenGenerator` is a bit difficult, but it seems to be a better solution, I will try 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
[GitHub] [shardingsphere] codecov-commenter commented on pull request #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#issuecomment-660460845
# [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/6377?src=pr&el=h1) Report
> Merging [#6377](https://codecov.io/gh/apache/shardingsphere/pull/6377?src=pr&el=desc) into [master](https://codecov.io/gh/apache/shardingsphere/commit/7311d0b306348fad4de3c43c72ee19c7444a3a67&el=desc) will **decrease** coverage by `0.08%`.
> The diff coverage is `30.00%`.
[](https://codecov.io/gh/apache/shardingsphere/pull/6377?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6377 +/- ##
============================================
- Coverage 52.54% 52.46% -0.09%
Complexity 453 453
============================================
Files 1224 1225 +1
Lines 21804 21858 +54
Branches 3887 3904 +17
============================================
+ Hits 11458 11468 +10
- Misses 9608 9643 +35
- Partials 738 747 +9
```
| [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/6377?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...che/shardingsphere/sharding/rule/ShardingRule.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9zaGFyZGluZy9ydWxlL1NoYXJkaW5nUnVsZS5qYXZh) | `67.54% <0.00%> (-1.83%)` | `0.00 <0.00> (ø)` | |
| [...on/engine/InsertClauseShardingConditionEngine.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctcm91dGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3NoYXJkaW5nL3JvdXRlL2VuZ2luZS9jb25kaXRpb24vZW5naW5lL0luc2VydENsYXVzZVNoYXJkaW5nQ29uZGl0aW9uRW5naW5lLmphdmE=) | `45.65% <0.00%> (-4.35%)` | `7.00 <0.00> (ø)` | |
| [...phere/infra/rewrite/context/SQLRewriteContext.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtaW5mcmEvc2hhcmRpbmdzcGhlcmUtaW5mcmEtcmV3cml0ZS9zaGFyZGluZ3NwaGVyZS1pbmZyYS1yZXdyaXRlLWVuZ2luZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvaW5mcmEvcmV3cml0ZS9jb250ZXh0L1NRTFJld3JpdGVDb250ZXh0LmphdmE=) | `70.58% <0.00%> (-9.42%)` | `0.00 <0.00> (ø)` | |
| [...der/segment/insert/values/InsertSelectContext.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWJpbmRlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9iaW5kZXIvc2VnbWVudC9pbnNlcnQvdmFsdWVzL0luc2VydFNlbGVjdENvbnRleHQuamF2YQ==) | `0.00% <0.00%> (ø)` | `0.00 <0.00> (?)` | |
| [.../sql/parser/sql/statement/dml/InsertStatement.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLXN0YXRlbWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9zcWwvc3RhdGVtZW50L2RtbC9JbnNlcnRTdGF0ZW1lbnQuamF2YQ==) | `84.21% <0.00%> (-4.68%)` | `0.00 <0.00> (ø)` | |
| [...r/binder/statement/dml/InsertStatementContext.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWJpbmRlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9iaW5kZXIvc3RhdGVtZW50L2RtbC9JbnNlcnRTdGF0ZW1lbnRDb250ZXh0LmphdmE=) | `72.09% <18.18%> (-18.54%)` | `0.00 <0.00> (ø)` | |
| [...lidator/impl/ShardingInsertStatementValidator.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctcm91dGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3NoYXJkaW5nL3JvdXRlL2VuZ2luZS92YWxpZGF0b3IvaW1wbC9TaGFyZGluZ0luc2VydFN0YXRlbWVudFZhbGlkYXRvci5qYXZh) | `70.58% <37.50%> (-29.42%)` | `0.00 <0.00> (ø)` | |
| [.../sharding/route/engine/ShardingRouteDecorator.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctcm91dGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3NoYXJkaW5nL3JvdXRlL2VuZ2luZS9TaGFyZGluZ1JvdXRlRGVjb3JhdG9yLmphdmE=) | `74.13% <40.00%> (-4.44%)` | `1.00 <0.00> (ø)` | |
| [.../route/engine/type/ShardingRouteEngineFactory.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctcm91dGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3NoYXJkaW5nL3JvdXRlL2VuZ2luZS90eXBlL1NoYXJkaW5nUm91dGVFbmdpbmVGYWN0b3J5LmphdmE=) | `80.95% <50.00%> (-5.54%)` | `0.00 <0.00> (ø)` | |
| [...te/context/ShardingSQLRewriteContextDecorator.java](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctcmV3cml0ZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc2hhcmRpbmcvcmV3cml0ZS9jb250ZXh0L1NoYXJkaW5nU1FMUmV3cml0ZUNvbnRleHREZWNvcmF0b3IuamF2YQ==) | `93.75% <87.50%> (-6.25%)` | `2.00 <0.00> (ø)` | |
| ... and [3 more](https://codecov.io/gh/apache/shardingsphere/pull/6377/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/6377?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/6377?src=pr&el=footer). Last update [7311d0b...033e183](https://codecov.io/gh/apache/shardingsphere/pull/6377?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456851487
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-binder/src/main/java/org/apache/shardingsphere/sql/parser/binder/statement/dml/InsertStatementContext.java
##########
@@ -78,6 +83,19 @@ public InsertStatementContext(final SchemaMetaData schemaMetaData, final List<Ob
return result;
}
+ private Optional<InsertSelectContext> getInsertSelectContext(final SchemaMetaData schemaMetaData, final String sql,
+ final List<Object> parameters, final AtomicInteger parametersOffset) {
+ if (!getSqlStatement().getInsertSelect().isPresent()) {
+ return Optional.empty();
+ }
+ SubquerySegment insertSelectSegment = getSqlStatement().getInsertSelect().get();
+ String selectSql = sql.substring(insertSelectSegment.getStartIndex(), insertSelectSegment.getStopIndex() + 1);
Review comment:
@tristaZero This suggestion sounds great. In the sql parsing phase, parse the required sql fragments, which is more elegant than passing parameters. 👍
I will try to use parsing way to remove sql parameters.
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456852765
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/impl/ShardingInsertStatementValidator.java
##########
@@ -35,9 +38,15 @@
@Override
public void validate(final ShardingRule shardingRule, final InsertStatement sqlStatement, final List<Object> parameters) {
Optional<OnDuplicateKeyColumnsSegment> onDuplicateKeyColumnsSegment = sqlStatement.getOnDuplicateKeyColumns();
- if (onDuplicateKeyColumnsSegment.isPresent() && isUpdateShardingKey(shardingRule, onDuplicateKeyColumnsSegment.get(), sqlStatement.getTable().getTableName().getIdentifier().getValue())) {
+ 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.");
}
+ Optional<SubquerySegment> insertSelectSegment = sqlStatement.getInsertSelect();
+ if (insertSelectSegment.isPresent() && isContainsKeyGenerateStrategy(shardingRule, tableName)
+ && !isContainsKeyGenerateColumn(shardingRule, sqlStatement.getColumns(), tableName)) {
+ throw new ShardingSphereException("INSERT INTO .... SELECT can not support key generate column.");
Review comment:
> This exception looks not clear enough, how about `INSERT INTO .... SELECT can not support applying keyGenerator to absent generateKeyColumn.` ?
This prompt is better. 👍
----------------------------------------------------------------
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 closed pull request #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu closed pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456796446
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-binder/src/main/java/org/apache/shardingsphere/sql/parser/binder/statement/dml/InsertStatementContext.java
##########
@@ -78,6 +83,19 @@ public InsertStatementContext(final SchemaMetaData schemaMetaData, final List<Ob
return result;
}
+ private Optional<InsertSelectContext> getInsertSelectContext(final SchemaMetaData schemaMetaData, final String sql,
+ final List<Object> parameters, final AtomicInteger parametersOffset) {
+ if (!getSqlStatement().getInsertSelect().isPresent()) {
+ return Optional.empty();
+ }
+ SubquerySegment insertSelectSegment = getSqlStatement().getInsertSelect().get();
+ String selectSql = sql.substring(insertSelectSegment.getStartIndex(), insertSelectSegment.getStopIndex() + 1);
Review comment:
Since `createProjection` needs `SQL`, here `SQL` is passed and handled. Maybe we consider parsing `SQL` into `sqlStatement`. It is another point, we could consider it later.
If `sqlStatement ` contained the text of `SQL`, we would not need to pass it through the parameter list.
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/context/ShardingSQLRewriteContextDecorator.java
##########
@@ -38,8 +42,15 @@
@Override
public void decorate(final ShardingRule shardingRule, final ConfigurationProperties props, final SQLRewriteContext sqlRewriteContext, final RouteContext routeContext) {
for (ParameterRewriter each : new ShardingParameterRewriterBuilder(shardingRule, routeContext).getParameterRewriters(sqlRewriteContext.getSchemaMetaData())) {
- if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlRewriteContext.getSqlStatementContext())) {
- each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlRewriteContext.getSqlStatementContext(), sqlRewriteContext.getParameters());
+ SQLStatementContext sqlStatementContext = sqlRewriteContext.getSqlStatementContext();
+ if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlStatementContext)) {
+ each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlStatementContext, sqlRewriteContext.getParameters());
+ }
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()
Review comment:
The content of this condition is a bit long, maybe it is better to exact a function for it.
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/impl/ShardingInsertStatementValidator.java
##########
@@ -35,9 +38,15 @@
@Override
public void validate(final ShardingRule shardingRule, final InsertStatement sqlStatement, final List<Object> parameters) {
Optional<OnDuplicateKeyColumnsSegment> onDuplicateKeyColumnsSegment = sqlStatement.getOnDuplicateKeyColumns();
- if (onDuplicateKeyColumnsSegment.isPresent() && isUpdateShardingKey(shardingRule, onDuplicateKeyColumnsSegment.get(), sqlStatement.getTable().getTableName().getIdentifier().getValue())) {
+ 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.");
}
+ Optional<SubquerySegment> insertSelectSegment = sqlStatement.getInsertSelect();
+ if (insertSelectSegment.isPresent() && isContainsKeyGenerateStrategy(shardingRule, tableName)
+ && !isContainsKeyGenerateColumn(shardingRule, sqlStatement.getColumns(), tableName)) {
+ throw new ShardingSphereException("INSERT INTO .... SELECT can not support key generate column.");
Review comment:
Do you think it is better to add estimation of `the table inserted and the table selected are the same or bind tables.`?
##########
File path: shardingsphere-infra/shardingsphere-infra-rewrite/shardingsphere-infra-rewrite-engine/src/main/java/org/apache/shardingsphere/infra/rewrite/context/SQLRewriteContext.java
##########
@@ -80,5 +80,8 @@ public void addSQLTokenGenerators(final Collection<SQLTokenGenerator> sqlTokenGe
*/
public void generateSQLTokens() {
sqlTokens.addAll(sqlTokenGenerators.generateSQLTokens(sqlStatementContext, parameters, schemaMetaData));
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()) {
Review comment:
It is indeed an easy way to handle the `select statement` in the `insert statement`. However, two points get my concerns,
1. IMO, `SQLRewriteContext` does not need to know which `sqlstatment` it is and whether it is required to generate SQL tokens. So maybe I know `isGenerateSQLToken(sqlStatementContext)` of each `tokenGenerator` is a more proper place to give some similar estimations. However, this way is a bit hard to do the same thing.
2. If we call `sqlTokenGenerators.generateSQLTokens()` twice, that means it runs `for each` twice internally. It is not efficient from my point.
I'd like to listen to your idea about it. :-)
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/type/ShardingRouteEngineFactory.java
##########
@@ -94,6 +96,15 @@ public static ShardingRouteEngine newInstance(final ShardingRule shardingRule, f
return getShardingRoutingEngine(shardingRule, sqlStatementContext, shardingConditions, tableNames, props);
}
+ private static Collection<String> getTableNames(final SQLStatementContext sqlStatementContext) {
+ Collection<String> tableNames = new LinkedList<>(sqlStatementContext.getTablesContext().getTableNames());
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()) {
Review comment:
`GetTableNames()` is supposed to be a function of `InsertStatementContext`. How abou moving these process to `InsertStatementContext` ?
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/context/ShardingSQLRewriteContextDecorator.java
##########
@@ -38,8 +42,15 @@
@Override
public void decorate(final ShardingRule shardingRule, final ConfigurationProperties props, final SQLRewriteContext sqlRewriteContext, final RouteContext routeContext) {
for (ParameterRewriter each : new ShardingParameterRewriterBuilder(shardingRule, routeContext).getParameterRewriters(sqlRewriteContext.getSchemaMetaData())) {
- if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlRewriteContext.getSqlStatementContext())) {
- each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlRewriteContext.getSqlStatementContext(), sqlRewriteContext.getParameters());
+ SQLStatementContext sqlStatementContext = sqlRewriteContext.getSqlStatementContext();
+ if (!sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(sqlStatementContext)) {
+ each.rewrite(sqlRewriteContext.getParameterBuilder(), sqlStatementContext, sqlRewriteContext.getParameters());
+ }
+ if (sqlStatementContext instanceof InsertStatementContext && null != ((InsertStatementContext) sqlStatementContext).getInsertSelectContext()
+ && !sqlRewriteContext.getParameters().isEmpty() && each.isNeedRewrite(((InsertStatementContext) sqlStatementContext).getInsertSelectContext().getSelectStatementContext())) {
+ StandardParameterBuilder parameterBuilder = sqlRewriteContext.getParameterBuilder() instanceof GroupedParameterBuilder
Review comment:
It is impossible to have many `values(), ()` in `INSERT SELECT`, isn't it?
If it is that case, there is no chance for `GroupedParameterBuilder` here, right?
Do you think no estimation for'GroupedParameterBuilder` is reasonable?
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-rewrite/src/test/resources/sharding/insert.xml
##########
@@ -132,6 +132,158 @@
<output sql="INSERT INTO t_account_1 VALUES (101, 2000, 'OK') ON DUPLICATE KEY UPDATE status = ?" parameters="OK_UPDATE" />
</rewrite-assertion>
+ <rewrite-assertion id="insert_select_with_all_columns_with_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_0 WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_all_columns_without_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_0 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_1 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_1 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_all_columns_with_sharding_column_for_literals" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = 20" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_0 WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_all_columns_without_sharding_column_for_literals" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = 20" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_0 WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = 20" />
+ <output sql="INSERT INTO t_account_1 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_1 WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_without_columns_with_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account SELECT account_id, amount, status FROM t_account WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ <output sql="INSERT INTO t_account_0 SELECT account_id, amount, status FROM t_account_0 WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_without_columns_without_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account SELECT account_id, amount, status FROM t_account WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_0 SELECT account_id, amount, status FROM t_account_0 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_1 SELECT account_id, amount, status FROM t_account_1 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_without_columns_with_sharding_column_for_literals" db-type="MySQL">
+ <input sql="INSERT INTO t_account SELECT account_id, amount, status FROM t_account WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = 20" />
+ <output sql="INSERT INTO t_account_0 SELECT account_id, amount, status FROM t_account_0 WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_without_columns_without_sharding_column_for_literals" db-type="MySQL">
+ <input sql="INSERT INTO t_account SELECT account_id, amount, status FROM t_account WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = 20" />
+ <output sql="INSERT INTO t_account_0 SELECT account_id, amount, status FROM t_account_0 WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = 20" />
+ <output sql="INSERT INTO t_account_1 SELECT account_id, amount, status FROM t_account_1 WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_with_all_columns_with_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail_0 WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_with_all_columns_without_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail_0 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_1 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail_1 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_with_all_columns_with_sharding_column_for_literals" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = VALUES(amount)" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail_0 WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = VALUES(amount)" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_with_all_columns_without_sharding_column_for_literals" db-type="MySQL">
+ <input sql="INSERT INTO t_account (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = VALUES(amount)" />
+ <output sql="INSERT INTO t_account_0 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail_0 WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = VALUES(amount)" />
+ <output sql="INSERT INTO t_account_1 (account_id, amount, status) SELECT account_id, amount, status FROM t_account_detail_1 WHERE amount = 1000 ON DUPLICATE KEY UPDATE amount = VALUES(amount)" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_without_columns_with_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account SELECT account_id, amount, status FROM t_account_detail WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ <output sql="INSERT INTO t_account_0 SELECT account_id, amount, status FROM t_account_detail_0 WHERE account_id = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="100, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_without_columns_without_sharding_column_for_parameters" db-type="MySQL">
+ <input sql="INSERT INTO t_account SELECT account_id, amount, status FROM t_account_detail WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_0 SELECT account_id, amount, status FROM t_account_detail_0 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ <output sql="INSERT INTO t_account_1 SELECT account_id, amount, status FROM t_account_detail_1 WHERE amount = ? ON DUPLICATE KEY UPDATE amount = ?" parameters="1000, 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_without_columns_with_sharding_column_for_literals" db-type="MySQL">
+ <input sql="INSERT INTO t_account SELECT account_id, amount, status FROM t_account_detail WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = 20" />
+ <output sql="INSERT INTO t_account_0 SELECT account_id, amount, status FROM t_account_detail_0 WHERE account_id = 100 ON DUPLICATE KEY UPDATE amount = 20" />
+ </rewrite-assertion>
+
+ <rewrite-assertion id="insert_select_with_binding_table_without_columns_without_sharding_column_for_literals" db-type="MySQL">
Review comment:
Oh, I like these test cases!
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/impl/ShardingInsertStatementValidator.java
##########
@@ -35,9 +38,15 @@
@Override
public void validate(final ShardingRule shardingRule, final InsertStatement sqlStatement, final List<Object> parameters) {
Optional<OnDuplicateKeyColumnsSegment> onDuplicateKeyColumnsSegment = sqlStatement.getOnDuplicateKeyColumns();
- if (onDuplicateKeyColumnsSegment.isPresent() && isUpdateShardingKey(shardingRule, onDuplicateKeyColumnsSegment.get(), sqlStatement.getTable().getTableName().getIdentifier().getValue())) {
+ 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.");
}
+ Optional<SubquerySegment> insertSelectSegment = sqlStatement.getInsertSelect();
+ if (insertSelectSegment.isPresent() && isContainsKeyGenerateStrategy(shardingRule, tableName)
+ && !isContainsKeyGenerateColumn(shardingRule, sqlStatement.getColumns(), tableName)) {
+ throw new ShardingSphereException("INSERT INTO .... SELECT can not support key generate column.");
Review comment:
This exception looks not clear enough, how about `INSERT INTO .... SELECT can not support applying keyGenerator to absent generateKeyColumn.` ?
----------------------------------------------------------------
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 #6377: support mysql insert select statement route and rewrite
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #6377:
URL: https://github.com/apache/shardingsphere/pull/6377#discussion_r456851487
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-binder/src/main/java/org/apache/shardingsphere/sql/parser/binder/statement/dml/InsertStatementContext.java
##########
@@ -78,6 +83,19 @@ public InsertStatementContext(final SchemaMetaData schemaMetaData, final List<Ob
return result;
}
+ private Optional<InsertSelectContext> getInsertSelectContext(final SchemaMetaData schemaMetaData, final String sql,
+ final List<Object> parameters, final AtomicInteger parametersOffset) {
+ if (!getSqlStatement().getInsertSelect().isPresent()) {
+ return Optional.empty();
+ }
+ SubquerySegment insertSelectSegment = getSqlStatement().getInsertSelect().get();
+ String selectSql = sql.substring(insertSelectSegment.getStartIndex(), insertSelectSegment.getStopIndex() + 1);
Review comment:
> Since `createProjection` needs `SQL`, here `SQL` is passed and handled. Maybe we consider parsing `SQL` into `sqlStatement`. It is another point, we could consider it later.
> If `sqlStatement ` contained the text of `SQL`, we would not need to pass it through the parameter list.
@tristaZero This suggestion sounds great. In the sql parsing phase, parse the required sql fragments, which is more elegant than passing parameters. 👍
I will try to use parsing way to remove sql parameters.
----------------------------------------------------------------
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