Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 31437 with keyword query error mysql #34163

Merged

Conversation

Yash-cor
Copy link
Contributor

@Yash-cor Yash-cor commented Dec 26, 2024

Fixes #31437

Changes proposed in this pull request:

Added DuplicateCommonTableExpressionAliasException to support with keyword binding.
This Issue is solved by the update in Sql binder in PR - #34141
Added WithSegmentBinderTest
Added with clause test in SQLBinderIT


Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.
  • I have updated the Release Notes of the current development version. For more details, see Update Release Note

@strongduanmu
Copy link
Member

Hi @Yash-cor, can you sync master branch, and add sql bind test case for with statement.

@Yash-cor
Copy link
Contributor Author

Hi @Yash-cor, can you sync master branch, and add sql bind test case for with statement.

Yeah sure

@Yash-cor Yash-cor marked this pull request as draft December 27, 2024 12:49
@Yash-cor Yash-cor marked this pull request as ready for review December 27, 2024 13:38
RELEASE-NOTES.md Outdated Show resolved Hide resolved
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()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this check happen in MySQL? If yes, please provide MySQL test results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the logic for UniqueCommonTableAlias check
This check happens in MySQL but in ShardingSphere this check was happening while the query is executed in MySQL database so I have added this check during the creation of Query Context.
Screenshot 2024-12-31 121634

@@ -47,6 +49,8 @@ public final class SQLStatementBinderContext {

private final SQLStatement sqlStatement;

private final Set<CaseInsensitiveString> commonTableExpressionsSegmentsUniqueAliases = new HashSet<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use Collection<CaseInsensitiveString> to replace Set<CaseInsensitiveString>.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, dou you think CaseInsensitiveSet is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I am using Collection<String> commonTableExpressionsSegmentsUniqueAliases = new CaseInsensitiveSet<>();

CaseInsensitiveSet will be better as it ease the overhead of creating the object in case of Set

private static final long serialVersionUID = -8206891094419297634L;

public UniqueCommonTableExpressionException(final String alias) {
super(XOpenSQLState.SYNTAX_ERROR, 500, "Not unique table/alias: '%s'", alias);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this exception consistent with the database?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If yes, please update the exception document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this Error is present in MySQL with MySQl error code 1066
and SQLState as 42000 so do I have to create this exception somewhere else

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add java doc for this class.

@@ -38,6 +39,8 @@ public final class MySQLDeleteStatement extends DeleteStatement implements MySQL

private ReturningSegment returningSegment;

private WithSegment withSegment;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide the documentation for MySQL DELETE WITH ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this class, you change the SQL parsing result, you need to add SQL Parser IT test to ensure that we handle withSegment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will make these changes once the logic for the CTE's Unique Alias Name is clear.

@Yash-cor Yash-cor marked this pull request as draft December 30, 2024 05:54
@Yash-cor Yash-cor marked this pull request as ready for review January 2, 2025 09:43
@Yash-cor Yash-cor requested a review from strongduanmu January 2, 2025 09:43
@Yash-cor Yash-cor marked this pull request as draft January 7, 2025 06:07
@Yash-cor Yash-cor marked this pull request as ready for review January 8, 2025 12:23
@Yash-cor Yash-cor requested a review from strongduanmu January 8, 2025 12:23
Copy link
Member

@strongduanmu strongduanmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks so great, thank you for your efforts on this PR, I believe the community will benefit from it.

@strongduanmu strongduanmu merged commit 5c6745e into apache:master Jan 9, 2025
147 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"with" sql not executed in the correct dataSource
2 participants