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.

Sign up for GitHub

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

Jump to bottom

Support for boolean primary MySQL queries #25424

Closed
wants to merge 21 commits into from
Closed

Support for boolean primary MySQL queries #25424

wants to merge 21 commits into from

Conversation

kanha-gupta
Copy link
Contributor

@kanha-gupta kanha-gupta commented Apr 30, 2023 •
edited

Changes proposed in this pull request:

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.

@kanha-gupta kanha-gupta changed the title Expression queries Support for boolean primary MySQL queries Apr 30, 2023
@kanha-gupta
Copy link
Contributor Author

kanha-gupta commented Apr 30, 2023 •
edited

@strongduanmu Respected sir, This PR works well (SQLNodeConverterEngineIT and XML tests are successful) but I am getting a maven error :
Error: Tests run: 265, Failures: 0, Errors: 8, Skipped: 0, Time elapsed: 6.686 s <<< FAILURE! - in org.apache.shardingsphere.test.it.rewrite.engine.scenario.EncryptSQLRewriterIT
Error: assertRewrite{SQLRewriteEngineTestParameters}[145] Time elapsed: 0.022 s <<< ERROR!
org.apache.shardingsphere.encrypt.exception.syntax.UnsupportedEncryptSQLException: The SQL clause IS NULL is unsupported in encrypt rule.

what is encrypt rule & how to add support for 'IS NULL' ?
I did this PR for code demonstration

@kanha-gupta
Copy link
Contributor Author

kanha-gupta commented May 1, 2023 •
edited

@strongduanmu Queries in Update.xml test file is giving the above error which follows scenario/encrypt/config/query-with-cipher.yaml configuration
while running EncryptSQLRewriterIT.java file.
can you please suggest whether changes are required in XML Files or In Code files or YAML file ?

@strongduanmu strongduanmu added this to the 5.4.0 milestone May 2, 2023
@kanha-gupta
Copy link
Contributor Author

@strongduanmu Please review, All database parsers have been optimised now.
Thank you

@strongduanmu
Copy link
Member

@strongduanmu Please review, All database parsers have been optimised now. Thank you

@kanha-gupta Good job, I will review this pr again.

strongduanmu
strongduanmu requested changes May 5, 2023
View reviewed changes
...rc/main/java/org/apache/shardingsphere/encrypt/rewrite/condition/EncryptConditionEngine.java
@@ -77,6 +77,8 @@ public final class EncryptConditionEngine {
SUPPORTED_COMPARE_OPERATOR.add("<=");
SUPPORTED_COMPARE_OPERATOR.add("IS");
Copy link
Member

Choose a reason for hiding this comment

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

Please remove useless IS operator.

...java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/MySQLStatementVisitor.java
@@ -441,7 +446,9 @@ public final ASTNode visitBooleanPrimary(final BooleanPrimaryContext ctx) {
ExpressionSegment right = new LiteralExpressionSegment(ctx.IS().getSymbol().getStopIndex() + 2, ctx.stop.getStopIndex(), rightText);
String text = ctx.start.getInputStream().getText(new Interval(ctx.start.getStartIndex(), ctx.stop.getStopIndex()));
ExpressionSegment left = (ExpressionSegment) visit(ctx.booleanPrimary());
String operator = "IS";
if (null != ctx.NULL()) {
right = null;
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 refer https://github.com/polardb/polardbx-sql/blob/cdd976850bdfc3264dff279198b6b06eb8641fbe/polardbx-optimizer/src/main/java/com/alibaba/polardbx/optimizer/parse/visitor/FastSqlToCalciteNodeVisitor.java#L5346-L5350 to extract is and is not as an operator, and keep the right to LiteralExpressionSegment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Surely sir, I tried it on my local system.
Keeping right to LiteralExpressionSegment is causing double NULL values ->
IS NULL NULL where :
IS NULL -> postfix operator
NULL -> LiteralExpressionSegment
Can you please suggest the fix for this ?

Copy link
Member

Choose a reason for hiding this comment

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

You can modify is null operator to is operator, is not null operator to is not operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strongduanmu Sir, I tried it. The problem I am encountering is that Calcite's SqlStdOperatorTable do not have "IS" as a function or PostFixOperator. Therefore While breaking "IS NULL" into "IS" as operator & "NULL" in right as LiteralExpressionSegment, It gives error "IS" is not supported.
Same problem is for "IS NOT NULL" as SqlStdOperatorTable doesnt support "IS NOT"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strongduanmu Therefore Please suggest a solution for it.
Thank you

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 refer https://github.com/polardb/polardbx-sql/blob/cdd976850bdfc3264dff279198b6b06eb8641fbe/polardbx-optimizer/src/main/java/com/alibaba/polardbx/optimizer/parse/visitor/FastSqlToCalciteNodeVisitor.java#L5346-L5350 to extract is and is not as an operator, and keep the right to LiteralExpressionSegment?

@kanha-gupta Can you take a look at this link? It provide an example to convert to sql node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strongduanmu Okay sir, I will update you with the progress :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@strongduanmu Sir, should I just revise CodeStyle ? in the example code, operator is set when Right is instanceof SqlNullExpr. Therefore I can understand that the Logic would stay same just like this current PR ?
Thank you

.../apache/shardingsphere/sql/parser/opengauss/visitor/statement/OpenGaussStatementVisitor.java
@@ -356,7 +356,7 @@ private BinaryOperationExpression createPatternMatchingOperationSegment(final AE

private BinaryOperationExpression createBinaryOperationSegment(final AExprContext ctx, final String operator) {
if ("IS".equalsIgnoreCase(operator)) {
ExpressionSegment left = (ExpressionSegment) visit(ctx.aExpr(0));
String ifOperator = operator;
Copy link
Member

Choose a reason for hiding this comment

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

What does ifOperator mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sir, I'll rename it. The variable helps in storing & changing String values when IF conditions are met.

@kanha-gupta kanha-gupta closed this May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@strongduanmu strongduanmu strongduanmu requested changes

Assignees
No one assigned
Labels
db: MySQL feature: SQL federation type: enhancement
Projects
None yet
Milestone
5.4.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kanha-gupta @strongduanmu