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

Add Doris grammar rules for RECOVER and tests #33436

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add Doris grammar rules for RECOVER and tests #33436

wants to merge 3 commits into from
+560 −1

Conversation

shamilv
Copy link

@shamilv shamilv commented Oct 28, 2024 •
edited
Loading

Fixes #31504 .

Changes proposed in this pull request:

  • Added changes necessary to parse Doris RECOVER statements including:
    • RECOVER PARTITION
    • RECOVER DATABASE
    • RECOVER TABLE
  • Added tests

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.

iamhucong
iamhucong reviewed Oct 31, 2024
View reviewed changes
parser/sql/dialect/doris/src/main/antlr4/imports/doris/DALStatement.g4 Outdated
@@ -411,6 +411,18 @@ restart
: RESTART
;

recoverDatabase
: RECOVER DATABASE databaseName (databaseId | AS newDatabaseName)?
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use rule-element-labels instead of the newDatabaseName rule.

Copy link
Author

Choose a reason for hiding this comment

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

Done

parser/sql/dialect/doris/src/main/antlr4/imports/doris/DALStatement.g4 Outdated
;

recoverPartition
: RECOVER PARTITION partitionName partitionId? (AS newPartitionName)? FROM tableName
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Author

Choose a reason for hiding this comment

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

Done

.../apache/shardingsphere/sql/parser/statement/core/statement/dal/RecoverDatabaseStatement.java Outdated
@Setter
public abstract class RecoverDatabaseStatement extends AbstractSQLStatement implements DDLStatement {

private String databaseName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you consider using the DatabaseSegment?

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to DatabaseSegment

...apache/shardingsphere/sql/parser/statement/core/statement/dal/RecoverPartitionStatement.java Outdated
Comment on lines 30 to 44
public abstract class RecoverPartitionStatement extends AbstractSQLStatement implements DDLStatement {

private String partitionName;

private String partitionId;

private String databaseName;

private String newPartitionName;

private String owner;

private String tableName;

}
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, use class types instead of the primitive types.

Copy link
Author

Choose a reason for hiding this comment

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

Replaced with the class types

...org/apache/shardingsphere/sql/parser/statement/doris/dal/DorisRecoverPartitionStatement.java Outdated
import org.apache.shardingsphere.sql.parser.statement.doris.DorisStatement;

/**
* Doris create database statement.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check the code comments.

Copy link
Author

Choose a reason for hiding this comment

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

Done

.../org/apache/shardingsphere/sql/parser/statement/doris/dal/DorisRecoverDatabaseStatement.java Outdated
/**
* Doris create database statement.
*/
public final class DorisRecoverDatabaseStatement extends RecoverDatabaseStatement implements DorisStatement {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to reuse this abstract class in other databases? If not, we should consider removing it.

Copy link
Author

Choose a reason for hiding this comment

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

Deleted the abstract class

@iamhucong
Copy link
Contributor

https://shardingsphere.apache.org/community/en/involved/conduct/code/#maintenance-conduct
@shamilv Could you please verify if MySQL supports these? You can refer to this document for further information.

Additionally, please update the release notes for 5.5.2-SNAPSHOT.

@strongduanmu
Copy link
Member

Hi @shamilv, can you take a look for @iamhucong suggestion?

@shamilv
Copy link
Author

shamilv commented Dec 11, 2024

Hi @strongduanmu
Sure. I'll push the changes this weekend.

@strongduanmu strongduanmu added this to the 5.5.2 milestone Dec 12, 2024
@shamilv shamilv force-pushed the 31504 branch 2 times, most recently from ef8a0c1 to 60fdf5e Compare December 26, 2024 17:04
@shamilv
Copy link
Author

shamilv commented Dec 26, 2024

https://shardingsphere.apache.org/community/en/involved/conduct/code/#maintenance-conduct @shamilv Could you please verify if MySQL supports these? You can refer to this document for further information.

Additionally, please update the release notes for 5.5.2-SNAPSHOT.

@iamhucong MySQL doesn't have a RECOVER SQL statement. Release notes are updated

@shamilv
Copy link
Author

shamilv commented Dec 26, 2024

Hi @iamhucong / @strongduanmu
Please review the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamhucong iamhucong iamhucong left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
db: Doris in: SQL parse type: enhancement
Projects
None yet
Milestone
5.5.2
Development

Successfully merging this pull request may close these issues.

Support parsing Doris sql
3 participants
@shamilv @iamhucong @strongduanmu