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

[#1231] Fix bugs in drop MySQL schemas #1235

Merged
merged 2 commits into from
Dec 25, 2023
Merged

Conversation

yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Dec 21, 2023

What changes were proposed in this pull request?

  • Allow dropping a database when cascase is true.
  • If the database is not empty and cascade is false, disable the drop operation.

Why are the changes needed?

Align the logic with Gravitino EntityStore.

Fix: #1231

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Add UT testDropMySQLDatabase

@yuqi1129 yuqi1129 self-assigned this Dec 21, 2023
if (cascade) {
throw new UnsupportedOperationException(
"MySQL does not support CASCADE option for DROP DATABASE.");
return dropDatabaseSql;
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to handle database not empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the condition !cascade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the condition !cascade?

No, MySQL will automatically delete all tables in the database when it is removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why @Clearvive said that MySQL doesn't support cascading drop, have you verified with different MySQL versions, is it a consistent behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MySQL does support cascading deletion of schema, execute drop database {database_name} will drop all tables under it.

@Clearvive , Please check it.

@jerryshao
Copy link
Contributor

@Clearvive can you please help to review?

@yuqi1129
Copy link
Contributor Author

@Clearvive can you please help to review?

@Clearvive , could you please assist in viewing this? thanks

Clearvive
Clearvive previously approved these changes Dec 25, 2023
Copy link
Contributor

@Clearvive Clearvive left a comment

Choose a reason for hiding this comment

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

LGTM

@Clearvive Clearvive self-requested a review December 25, 2023 06:14
@Clearvive Clearvive dismissed their stale review December 25, 2023 06:14

re review

@jerryshao jerryshao added need backport Issues that need to backport to another branch branch-0.3 labels Dec 25, 2023
@jerryshao jerryshao merged commit 3ece627 into apache:main Dec 25, 2023
11 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 25, 2023
### What changes were proposed in this pull request?

- Allow dropping a database when `cascase` is `true`.
- If the database is not empty and `cascade` is `false`, disable the
drop operation.

### Why are the changes needed?

Align the logic with Gravitino EntityStore.

Fix: #1231 

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

Add UT `testDropMySQLDatabase`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need backport Issues that need to backport to another branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug report] Drop schema with false cascade goes unexpected
5 participants