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:
[email protected]
