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/10/15 04:55:18 UTC
[GitHub] [shardingsphere] strongduanmu opened a new pull request #7796: fix multi-table validation error for DML modify statements
strongduanmu opened a new pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796
Ref #7731.
Changes proposed in this pull request:
- optimize the validator check prompt message
- fix multi-table validation error for DML modify statements
----------------------------------------------------------------
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 #7796: fix multi-table validation error for DML modify statements
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796#discussion_r505584605
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/dml/ShardingDMLStatementValidator.java
##########
@@ -31,11 +34,14 @@
/**
* Validate multiple table.
*
+ * @param shardingRule sharding rule
* @param sqlStatementContext sqlStatementContext
+ * @param sql sql
*/
- protected void validateMultipleTable(final SQLStatementContext<T> sqlStatementContext) {
- if (1 != ((TableAvailable) sqlStatementContext).getAllTables().size()) {
- throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", sqlStatementContext.getSqlStatement());
+ protected void validateMultipleTable(final ShardingRule shardingRule, final SQLStatementContext<T> sqlStatementContext, final String sql) {
Review comment:
> From my perspective, It is unnecessary to have SQL as an input parameter. Precisely speaking, It is a little heavy to pass SQL just for an exception. Maybe Cannot support such a Multiple-Table query for '$tableNames' is enough. What do you think?
@tristaZero I agree with you that the inclusion of SQL in the exception is indeed a bit verbose, especially when the SQL is very complex.
----------------------------------------------------------------
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 #7796: fix multi-table validation error for DML modify statements
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796
----------------------------------------------------------------
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 #7796: fix multi-table validation error for DML modify statements
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796#discussion_r507016688
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/dml/ShardingDMLStatementValidator.java
##########
@@ -18,24 +18,38 @@
package org.apache.shardingsphere.sharding.route.engine.validator.dml;
import org.apache.shardingsphere.infra.binder.statement.SQLStatementContext;
-import org.apache.shardingsphere.infra.binder.type.TableAvailable;
import org.apache.shardingsphere.infra.exception.ShardingSphereException;
import org.apache.shardingsphere.sharding.route.engine.validator.ShardingStatementValidator;
+import org.apache.shardingsphere.sharding.rule.ShardingRule;
import org.apache.shardingsphere.sql.parser.sql.common.statement.SQLStatement;
+import java.util.Collection;
+import java.util.LinkedList;
+
/**
* Sharding dml statement validator.
*/
public abstract class ShardingDMLStatementValidator<T extends SQLStatement> implements ShardingStatementValidator<T> {
/**
- * Validate multiple table.
+ * Validate sharding multiple table.
*
+ * @param shardingRule sharding rule
* @param sqlStatementContext sqlStatementContext
*/
- protected void validateMultipleTable(final SQLStatementContext<T> sqlStatementContext) {
- if (1 != ((TableAvailable) sqlStatementContext).getAllTables().size()) {
- throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", sqlStatementContext.getSqlStatement());
+ protected void validateShardingMultipleTable(final ShardingRule shardingRule, final SQLStatementContext<T> sqlStatementContext) {
+ Collection<String> tableNames = sqlStatementContext.getTablesContext().getTableNames();
+ Collection<String> shardingTableNames = shardingRule.getShardingLogicTableNames(tableNames);
+ if ((1 == shardingTableNames.size() || shardingRule.isAllBindingTables(shardingTableNames)) && !isAllValidTables(shardingRule, tableNames)) {
+ throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", tableNames);
Review comment:
What's the string output of `tableNames`? Do you think that will be `Cannot support Multiple-Table for 'tb1tb2tb3'`?
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/dml/ShardingDMLStatementValidator.java
##########
@@ -18,24 +18,38 @@
package org.apache.shardingsphere.sharding.route.engine.validator.dml;
import org.apache.shardingsphere.infra.binder.statement.SQLStatementContext;
-import org.apache.shardingsphere.infra.binder.type.TableAvailable;
import org.apache.shardingsphere.infra.exception.ShardingSphereException;
import org.apache.shardingsphere.sharding.route.engine.validator.ShardingStatementValidator;
+import org.apache.shardingsphere.sharding.rule.ShardingRule;
import org.apache.shardingsphere.sql.parser.sql.common.statement.SQLStatement;
+import java.util.Collection;
+import java.util.LinkedList;
+
/**
* Sharding dml statement validator.
*/
public abstract class ShardingDMLStatementValidator<T extends SQLStatement> implements ShardingStatementValidator<T> {
/**
- * Validate multiple table.
+ * Validate sharding multiple table.
*
+ * @param shardingRule sharding rule
* @param sqlStatementContext sqlStatementContext
*/
- protected void validateMultipleTable(final SQLStatementContext<T> sqlStatementContext) {
- if (1 != ((TableAvailable) sqlStatementContext).getAllTables().size()) {
- throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", sqlStatementContext.getSqlStatement());
+ protected void validateShardingMultipleTable(final ShardingRule shardingRule, final SQLStatementContext<T> sqlStatementContext) {
+ Collection<String> tableNames = sqlStatementContext.getTablesContext().getTableNames();
+ Collection<String> shardingTableNames = shardingRule.getShardingLogicTableNames(tableNames);
+ if ((1 == shardingTableNames.size() || shardingRule.isAllBindingTables(shardingTableNames)) && !isAllValidTables(shardingRule, tableNames)) {
Review comment:
Do you think `if ( !isAllValidTables(shardingRule, tableNames))` is enough?
----------------------------------------------------------------
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 #7796: fix multi-table validation error for DML modify statements
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796#discussion_r507019709
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/dml/ShardingDMLStatementValidator.java
##########
@@ -18,24 +18,38 @@
package org.apache.shardingsphere.sharding.route.engine.validator.dml;
import org.apache.shardingsphere.infra.binder.statement.SQLStatementContext;
-import org.apache.shardingsphere.infra.binder.type.TableAvailable;
import org.apache.shardingsphere.infra.exception.ShardingSphereException;
import org.apache.shardingsphere.sharding.route.engine.validator.ShardingStatementValidator;
+import org.apache.shardingsphere.sharding.rule.ShardingRule;
import org.apache.shardingsphere.sql.parser.sql.common.statement.SQLStatement;
+import java.util.Collection;
+import java.util.LinkedList;
+
/**
* Sharding dml statement validator.
*/
public abstract class ShardingDMLStatementValidator<T extends SQLStatement> implements ShardingStatementValidator<T> {
/**
- * Validate multiple table.
+ * Validate sharding multiple table.
*
+ * @param shardingRule sharding rule
* @param sqlStatementContext sqlStatementContext
*/
- protected void validateMultipleTable(final SQLStatementContext<T> sqlStatementContext) {
- if (1 != ((TableAvailable) sqlStatementContext).getAllTables().size()) {
- throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", sqlStatementContext.getSqlStatement());
+ protected void validateShardingMultipleTable(final ShardingRule shardingRule, final SQLStatementContext<T> sqlStatementContext) {
+ Collection<String> tableNames = sqlStatementContext.getTablesContext().getTableNames();
+ Collection<String> shardingTableNames = shardingRule.getShardingLogicTableNames(tableNames);
+ if ((1 == shardingTableNames.size() || shardingRule.isAllBindingTables(shardingTableNames)) && !isAllValidTables(shardingRule, tableNames)) {
+ throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", tableNames);
Review comment:
> What's the string output of `tableNames`? Do you think that will be `Cannot support Multiple-Table for 'tb1tb2tb3'`?
@tristaZero The error message may like Cannot support Multiple-Table for '[user, order, order_item]'.
----------------------------------------------------------------
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 #7796: fix multi-table validation error for DML modify statements
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796#discussion_r505475984
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/dml/ShardingDMLStatementValidator.java
##########
@@ -31,11 +34,14 @@
/**
* Validate multiple table.
*
+ * @param shardingRule sharding rule
* @param sqlStatementContext sqlStatementContext
+ * @param sql sql
*/
- protected void validateMultipleTable(final SQLStatementContext<T> sqlStatementContext) {
- if (1 != ((TableAvailable) sqlStatementContext).getAllTables().size()) {
- throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", sqlStatementContext.getSqlStatement());
+ protected void validateMultipleTable(final ShardingRule shardingRule, final SQLStatementContext<T> sqlStatementContext, final String sql) {
Review comment:
1. From my perspective, It is unnecessary to have SQL as an input parameter. Precisely speaking, It is a little heavy to pass SQL just for an exception. Maybe `Cannot support such a Multiple-Table query for '$tableNames'` is enough. What do you think?
2. The constrictions for multi-tables looks like incomplete? How about broadcast tables or the single table in the same instance with sharding tables (Maybe a postValidate()?).
----------------------------------------------------------------
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 #7796: fix multi-table validation error for DML modify statements
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796#discussion_r505621119
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/dml/ShardingDMLStatementValidator.java
##########
@@ -31,11 +34,14 @@
/**
* Validate multiple table.
*
+ * @param shardingRule sharding rule
* @param sqlStatementContext sqlStatementContext
+ * @param sql sql
*/
- protected void validateMultipleTable(final SQLStatementContext<T> sqlStatementContext) {
- if (1 != ((TableAvailable) sqlStatementContext).getAllTables().size()) {
- throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", sqlStatementContext.getSqlStatement());
+ protected void validateMultipleTable(final ShardingRule shardingRule, final SQLStatementContext<T> sqlStatementContext, final String sql) {
Review comment:
> The constrictions for multi-tables looks like incomplete? How about broadcast tables or the single table in the same instance with sharding tables (Maybe a postValidate()?).
@tristaZero I'm very sorry, I will support the check of broadcast tables and single tables.
In addition, I think these check should be placed in preValidate, because they should be executed before routing. 🤔
----------------------------------------------------------------
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 #7796: fix multi-table validation error for DML modify statements
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on a change in pull request #7796:
URL: https://github.com/apache/shardingsphere/pull/7796#discussion_r507020493
##########
File path: shardingsphere-features/shardingsphere-sharding/shardingsphere-sharding-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/validator/dml/ShardingDMLStatementValidator.java
##########
@@ -18,24 +18,38 @@
package org.apache.shardingsphere.sharding.route.engine.validator.dml;
import org.apache.shardingsphere.infra.binder.statement.SQLStatementContext;
-import org.apache.shardingsphere.infra.binder.type.TableAvailable;
import org.apache.shardingsphere.infra.exception.ShardingSphereException;
import org.apache.shardingsphere.sharding.route.engine.validator.ShardingStatementValidator;
+import org.apache.shardingsphere.sharding.rule.ShardingRule;
import org.apache.shardingsphere.sql.parser.sql.common.statement.SQLStatement;
+import java.util.Collection;
+import java.util.LinkedList;
+
/**
* Sharding dml statement validator.
*/
public abstract class ShardingDMLStatementValidator<T extends SQLStatement> implements ShardingStatementValidator<T> {
/**
- * Validate multiple table.
+ * Validate sharding multiple table.
*
+ * @param shardingRule sharding rule
* @param sqlStatementContext sqlStatementContext
*/
- protected void validateMultipleTable(final SQLStatementContext<T> sqlStatementContext) {
- if (1 != ((TableAvailable) sqlStatementContext).getAllTables().size()) {
- throw new ShardingSphereException("Cannot support Multiple-Table for '%s'.", sqlStatementContext.getSqlStatement());
+ protected void validateShardingMultipleTable(final ShardingRule shardingRule, final SQLStatementContext<T> sqlStatementContext) {
+ Collection<String> tableNames = sqlStatementContext.getTablesContext().getTableNames();
+ Collection<String> shardingTableNames = shardingRule.getShardingLogicTableNames(tableNames);
+ if ((1 == shardingTableNames.size() || shardingRule.isAllBindingTables(shardingTableNames)) && !isAllValidTables(shardingRule, tableNames)) {
Review comment:
> Do you think `if ( !isAllValidTables(shardingRule, tableNames))` is enough?
@tristaZero In order to prevent it from affecting all the `RoutineEngine`, temporarily use the condition `(1 == shardingTableNames.size() || shardingRule.isAllBindingTables(shardingTableNames))` to ensure that only the ShardingStandardRoutingEngine is checked.
After I investigate the logic of all `RouteEngine`, I will optimize this logic in the future.
----------------------------------------------------------------
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