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 parse opengauss create index #28291

Closed
wants to merge 13 commits into from
Closed

support parse opengauss create index #28291

wants to merge 13 commits into from

Conversation

tmyVvit
Copy link

@tmyVvit tmyVvit commented Aug 28, 2023 •
edited
Loading

Fixes #27862

Changes proposed in this pull request:

  • Support parse openGauss create index with partition and tablespace sql
  • Support parse openGauss alter index move partition sql
  • Support parse openGauss alter index rename partition sql

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.

@strongduanmu strongduanmu added this to the 5.4.1 milestone Aug 28, 2023
@tmyVvit
Copy link
Author

tmyVvit commented Aug 30, 2023

hello @strongduanmu, can you give advice how to fix the failing checks. I find the log shows zookeeper session closed. But I don't know it is anything wrong with the code changes or just the environment issue.
image

@strongduanmu
Copy link
Member

Can you run GeneralDQLE2EIT on your local machine?

@tmyVvit
Copy link
Author

tmyVvit commented Sep 2, 2023

I can run GeneralDQLE2EIT, and there is no error on my local

@strongduanmu
Copy link
Member

I can run GeneralDQLE2EIT, and there is no error on my local

You can modify it-env.props according to fail e2e test case.

@tmyVvit
Copy link
Author

tmyVvit commented Sep 5, 2023

I have run the e2e test, and everything is OK. I run this: ./mvnw -nsu -B install -f test/e2e/sql/pom.xml -Dspotless.apply.skip=true -Dit.cluster.env.type=DOCKER -Dit.cluster.adapters=proxy -Dit.run.modes=Cluster -Dit.run.additional.cases=false -Dit.scenarios=tbl -Dit.cluster.databases=MySQL according to the failed case.
Can you help to re-trigger the checks?

@taojintianxia
Copy link
Contributor

this PR will migrate to milestone 5.4.2

@taojintianxia taojintianxia modified the milestones: 5.4.1, 5.4.2 Sep 28, 2023
@strongduanmu
Copy link
Member

Hi @tmyVvit, can you merge master branch to keep code consistent with the master branch?

@tmyVvit
Copy link
Author

tmyVvit commented Oct 10, 2023 via email

strongduanmu
strongduanmu requested changes Oct 18, 2023
View reviewed changes
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.

Hi @tmyVvit, thank you for your great contribution. There are a few minor issues you need to take a look at.

parser/sql/dialect/opengauss/src/main/antlr4/imports/opengauss/DDLStatement.g4 Outdated
@@ -348,6 +361,7 @@ partitionCmd

alterIndexDefinitionClause
: renameIndexSpecification | alterIndexDependsOnExtension | alterIndexSetTableSpace | alterTableCmds | indexPartitionCmd
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 write the rules one by one? According to the code specification, when there are more than 5 branches, a rule needs to be written one line.

.../apache/shardingsphere/sql/parser/opengauss/visitor/statement/OpenGaussStatementVisitor.java Outdated
@Override
public ASTNode visitCreateIndexPartitionClause(final OpenGaussStatementParser.CreateIndexPartitionClauseContext ctx) {
IndexPartitionTypeEnum indexPartitionType = null;
if (ctx.LOCAL() != null || ctx.GLOBAL() != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Please put null codition on the left side.

.../apache/shardingsphere/sql/parser/opengauss/visitor/statement/OpenGaussStatementVisitor.java Outdated
}

@Override
public ASTNode visitIndexPartitionElemList(final OpenGaussStatementParser.IndexPartitionElemListContext ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

Please import subclass directly.

.../apache/shardingsphere/sql/parser/opengauss/visitor/statement/OpenGaussStatementVisitor.java Outdated
}

@Override
public ASTNode visitIndexPartitionElem(final OpenGaussStatementParser.IndexPartitionElemContext ctx) {
Copy link
Member

Choose a reason for hiding this comment

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

Please import subclass directly.

.../apache/shardingsphere/sql/parser/sql/common/segment/ddl/partition/PartitionNameSegment.java Outdated
private final int stopIndex;

private final IdentifierValue identifier;

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 blank line.

...hardingsphere/sql/parser/sql/dialect/statement/opengauss/segment/IndexPartitionTypeEnum.java

package org.apache.shardingsphere.sql.parser.sql.dialect.statement.opengauss.segment;

public enum IndexPartitionTypeEnum {
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 javadoc for this class.

...hardingsphere/sql/parser/sql/dialect/statement/opengauss/segment/IndexPartitionsSegment.java Outdated

@RequiredArgsConstructor
@Getter
public class IndexPartitionsSegment implements AlterDefinitionSegment {
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 final and javadoc for this class.

.../shardingsphere/sql/parser/sql/dialect/statement/opengauss/segment/MovePartitionSegment.java Outdated
@RequiredArgsConstructor
@Getter
@Setter
public class MovePartitionSegment implements SQLSegment {
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 final and javadoc for this class.

...hardingsphere/sql/parser/sql/dialect/statement/opengauss/segment/RenamePartitionSegment.java Outdated Show resolved Hide resolved
...sphere/test/it/sql/parser/internal/asserts/statement/ddl/impl/AlterIndexStatementAssert.java Outdated
} else {
assertTrue(ogActual.getMovePartition().isPresent(), assertContext.getText("Actual move partition should exist."));
MovePartitionSegment actualMovePartition = ogActual.getMovePartition().get();
// assert partition
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 inline comment.

@tmyVvit
Copy link
Author

tmyVvit commented Oct 18, 2023

Hi @strongduanmu, thanks your suggestion, I have fixed the issues

@strongduanmu
Copy link
Member

Since this pr has long time no response, I will close it. You can submit new pr when you are free.

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: openGauss in: SQL parse type: enhancement
Projects
None yet
Milestone
5.5.0
Development

Successfully merging this pull request may close these issues.

Support parsing openGauss CREATE sql
3 participants
@tmyVvit @strongduanmu @taojintianxia