strongduanmu commented on code in PR #34163:
URL: https://github.com/apache/shardingsphere/pull/34163#discussion_r1899231597
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/from/type/SimpleTableSegmentBinder.java:
##########
@@ -85,6 +85,7 @@ public static SimpleTableSegment bind(final
SimpleTableSegment segment, final SQ
IdentifierValue databaseName = getDatabaseName(segment, binderContext);
IdentifierValue schemaName = getSchemaName(segment, binderContext,
databaseName);
IdentifierValue tableName = segment.getTableName().getIdentifier();
+
Review Comment:
Please
remove this useless blank line.
##########
infra/common/src/main/java/org/apache/shardingsphere/infra/exception/kernel/syntax/UniqueCommonTableExpressionException.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.infra.exception.kernel.syntax;
+
+import
org.apache.shardingsphere.infra.exception.core.external.sql.sqlstate.XOpenSQLState;
+import
org.apache.shardingsphere.infra.exception.core.external.sql.type.kernel.category.SyntaxSQLException;
+
+public final class UniqueCommonTableExpressionException extends
SyntaxSQLException {
Review Comment:
Please add java doc for this class.
##########
RELEASE-NOTES.md:
##########
@@ -140,7 +143,6 @@
1. Pipeline: Support page query for inventory dumper and data consistency
streaming query
1. Pipeline: Use case-insensitive identifiers to enhance the table metadata
loader
1. Pipeline: Support primary key columns ordering for standard pipeline table
metadata loader
-1. Sharding: Optimize sharding table index name rewriting rules and remove
unnecessary suffix rewriting -
[#31171](https://github.com/apache/shardingsphere/issues/31171)
Review Comment:
Why remove this release note?
##########
infra/binder/src/test/java/org/apache/shardingsphere/infra/binder/with/WithSegmentBinderTest.java:
##########
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.infra.binder.with;
+
+import com.cedarsoftware.util.CaseInsensitiveMap.CaseInsensitiveString;
+import
org.apache.shardingsphere.infra.binder.engine.segment.dml.from.context.type.SimpleTableSegmentBinderContext;
+import
org.apache.shardingsphere.infra.binder.engine.segment.dml.with.WithSegmentBinder;
+import
org.apache.shardingsphere.infra.binder.engine.statement.SQLStatementBinderContext;
+import org.apache.shardingsphere.infra.hint.HintValueContext;
+import org.apache.shardingsphere.infra.metadata.ShardingSphereMetaData;
+import
org.apache.shardingsphere.infra.metadata.database.schema.model.ShardingSphereColumn;
+import
org.apache.shardingsphere.infra.metadata.database.schema.model.ShardingSphereSchema;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.dml.column.ColumnSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.complex.CommonTableExpressionSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.subquery.SubquerySegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.dml.item.ColumnProjectionSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.dml.item.ProjectionsSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.dml.item.ShorthandProjectionSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.generic.AliasSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.generic.WithSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.generic.table.SimpleTableSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.segment.generic.table.TableNameSegment;
+import
org.apache.shardingsphere.sql.parser.statement.core.value.identifier.IdentifierValue;
+import
org.apache.shardingsphere.sql.parser.statement.mysql.dml.MySQLSelectStatement;
+import org.junit.jupiter.api.Test;
+
+import java.sql.Types;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.LinkedList;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
+
+public class WithSegmentBinderTest {
Review Comment:
Please add sql bind test in SQLBinderIT instead of this unit test.
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/expression/type/SubquerySegmentBinder.java:
##########
@@ -46,7 +48,13 @@ public static SubquerySegment bind(final SubquerySegment
segment, final SQLState
SQLStatementBinderContext selectBinderContext =
new SQLStatementBinderContext(binderContext.getMetaData(),
binderContext.getCurrentDatabaseName(), binderContext.getHintValueContext(),
segment.getSelect());
selectBinderContext.getExternalTableBinderContexts().putAll(binderContext.getExternalTableBinderContexts());
+
selectBinderContext.getCommonTableExpressionsSegmentsUniqueAliases().addAll(binderContext.getCommonTableExpressionsSegmentsUniqueAliases());
SelectStatement boundSelectStatement = new
SelectStatementBinder(outerTableBinderContexts).bind(segment.getSelect(),
selectBinderContext);
- return new SubquerySegment(segment.getStartIndex(),
segment.getStopIndex(), boundSelectStatement, segment.getText());
+ SubquerySegment result = new SubquerySegment(segment.getStartIndex(),
segment.getStopIndex(), boundSelectStatement, segment.getText());
+
selectBinderContext.getCommonTableExpressionsSegmentsUniqueAliases().forEach(each
-> {
+
ShardingSpherePreconditions.checkNotContains(binderContext.getCommonTableExpressionsSegmentsUniqueAliases(),
each, () -> new UniqueCommonTableExpressionException(each.toString()));
Review Comment:
Does this check happen in MySQL? If yes, please provide MySQL test results.
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/engine/segment/dml/with/WithSegmentBinder.java:
##########
@@ -61,12 +63,18 @@ public static WithSegment bind(final WithSegment segment,
final SQLStatementBind
for (CommonTableExpressionSegment each :
segment.getCommonTableExpressions()) {
CommonTableExpressionSegment boundCommonTableExpression =
CommonTableExpressionSegmentBinder.bind(each, binderContext,
segment.isRecursive());
boundCommonTableExpressions.add(boundCommonTableExpression);
+ if (each.getAliasName().isPresent()) {
+
ShardingSpherePreconditions.checkNotContains(binderContext.getCommonTableExpressionsSegmentsUniqueAliases(),
each.getAliasName(),
+ () -> new
UniqueCommonTableExpressionException(each.getAliasName().get()));
+
binderContext.getCommonTableExpressionsSegmentsUniqueAliases().add(new
CaseInsensitiveString(each.getAliasName().get()));
+ }
if (segment.isRecursive() && each.getAliasName().isPresent()) {
externalTableBinderContexts.removeAll(new
CaseInsensitiveString(each.getAliasName().get()));
}
bindWithColumns(each.getColumns(), boundCommonTableExpression);
each.getAliasName().ifPresent(optional ->
externalTableBinderContexts.put(new CaseInsensitiveString(optional),
createWithTableBinderContext(boundCommonTableExpression)));
}
+
Review Comment:
Please remove useless blank line.
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/engine/statement/SQLStatementBinderContext.java:
##########
@@ -47,6 +49,8 @@ public final class SQLStatementBinderContext {
private final SQLStatement sqlStatement;
+ private final Set<CaseInsensitiveString>
commonTableExpressionsSegmentsUniqueAliases = new HashSet<>();
Review Comment:
Please use `Collection<CaseInsensitiveString>` to replace
`Set<CaseInsensitiveString>`.
##########
parser/sql/statement/type/mysql/src/main/java/org/apache/shardingsphere/sql/parser/statement/mysql/dml/MySQLDeleteStatement.java:
##########
@@ -38,6 +39,8 @@ public final class MySQLDeleteStatement extends
DeleteStatement implements MySQL
private ReturningSegment returningSegment;
+ private WithSegment withSegment;
Review Comment:
For this class, you change the SQL parsing result, you need to add SQL
Parser IT test to ensure that we handle withSegment
##########
parser/sql/statement/type/mysql/src/main/java/org/apache/shardingsphere/sql/parser/statement/mysql/dml/MySQLDeleteStatement.java:
##########
@@ -38,6 +39,8 @@ public final class MySQLDeleteStatement extends
DeleteStatement implements MySQL
private ReturningSegment returningSegment;
+ private WithSegment withSegment;
Review Comment:
Can you provide the documentation for MySQL DELETE WITH ?
##########
infra/common/src/main/java/org/apache/shardingsphere/infra/exception/kernel/syntax/UniqueCommonTableExpressionException.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.infra.exception.kernel.syntax;
+
+import
org.apache.shardingsphere.infra.exception.core.external.sql.sqlstate.XOpenSQLState;
+import
org.apache.shardingsphere.infra.exception.core.external.sql.type.kernel.category.SyntaxSQLException;
+
+public final class UniqueCommonTableExpressionException extends
SyntaxSQLException {
+
+ private static final long serialVersionUID = -8206891094419297634L;
+
+ public UniqueCommonTableExpressionException(final String alias) {
+ super(XOpenSQLState.SYNTAX_ERROR, 500, "Not unique table/alias: '%s'",
alias);
Review Comment:
Is this exception consistent with the database?
##########
infra/common/src/main/java/org/apache/shardingsphere/infra/exception/kernel/syntax/UniqueCommonTableExpressionException.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.shardingsphere.infra.exception.kernel.syntax;
+
+import
org.apache.shardingsphere.infra.exception.core.external.sql.sqlstate.XOpenSQLState;
+import
org.apache.shardingsphere.infra.exception.core.external.sql.type.kernel.category.SyntaxSQLException;
+
+public final class UniqueCommonTableExpressionException extends
SyntaxSQLException {
+
+ private static final long serialVersionUID = -8206891094419297634L;
+
+ public UniqueCommonTableExpressionException(final String alias) {
+ super(XOpenSQLState.SYNTAX_ERROR, 500, "Not unique table/alias: '%s'",
alias);
Review Comment:
If yes, please update the exception document.
##########
infra/binder/src/main/java/org/apache/shardingsphere/infra/binder/engine/statement/SQLStatementBinderContext.java:
##########
@@ -47,6 +49,8 @@ public final class SQLStatementBinderContext {
private final SQLStatement sqlStatement;
+ private final Set<CaseInsensitiveString>
commonTableExpressionsSegmentsUniqueAliases = new HashSet<>();
Review Comment:
Besides, dou you think CaseInsensitiveSet 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
