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

feat: implement drop multiple tables #4085

Merged
merged 8 commits into from
Jun 4, 2024
Merged

Conversation

sarailQAQ
Copy link
Contributor

@sarailQAQ sarailQAQ commented May 31, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

close #4020

What's changed and what's your intention?

Implemet dropping multiple tables in the single statement.

If any tables do not exist without IF EXISTS, the statement fails with an error and don`t make any change, referencing MySQL.

It works for SQL requests, not ready for gRPC.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.

@sarailQAQ sarailQAQ requested a review from a team as a code owner May 31, 2024 12:54
@github-actions github-actions bot added the docs-not-required This change does not impact docs. label May 31, 2024
@WenyXu
Copy link
Member

WenyXu commented May 31, 2024

Would you like to add some sqlness tests in

DROP TABLE IF EXISTS foo;
create table foo (
host string,
ts timestamp DEFAULT '2023-04-29 00:00:00+00:00',
cpu double default 0,
TIME INDEX (ts),
PRIMARY KEY(host)
) engine=mito;
DROP TABLE IF EXISTS foo;
DROP TABLE IF EXISTS foo;

Including the cases of success, partial success, and failure

@WenyXu
Copy link
Member

WenyXu commented May 31, 2024

The Rustfmt and Clippy checks are failed, run make fmt and make clippy will help you to fix them

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 89.39394% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.14%. Comparing base (c0aed1d) to head (71d10bd).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4085      +/-   ##
==========================================
- Coverage   85.44%   85.14%   -0.30%     
==========================================
  Files         992      992              
  Lines      173377   173412      +35     
==========================================
- Hits       148141   147652     -489     
- Misses      25236    25760     +524     

@github-actions github-actions bot added docs-required This change requires docs update. and removed docs-not-required This change does not impact docs. labels May 31, 2024
@sarailQAQ
Copy link
Contributor Author

image
It seems runner does not have enough space? How to fix it?

@WenyXu
Copy link
Member

WenyXu commented Jun 2, 2024

image It seems runner does not have enough space? How to fix it?

We are still trying to fix it #4081

@WenyXu
Copy link
Member

WenyXu commented Jun 3, 2024

Hi @sarailQAQ let’s rebase this PR, the CI should be fixed

src/sql/src/statements/drop.rs Outdated Show resolved Hide resolved
src/sql/src/statements/drop.rs Outdated Show resolved Hide resolved
src/sql/src/statements/drop.rs Outdated Show resolved Hide resolved
src/operator/src/statement.rs Outdated Show resolved Hide resolved
src/operator/src/statement/ddl.rs Outdated Show resolved Hide resolved
src/operator/src/statement/ddl.rs Outdated Show resolved Hide resolved
@evenyag evenyag requested review from killme2008 and WenyXu June 4, 2024 08:06
@WenyXu WenyXu added this pull request to the merge queue Jun 4, 2024
Merged via the queue into GreptimeTeam:main with commit 98c19ed Jun 4, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-required This change requires docs update.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support to drop multiple tables
3 participants