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

refactor: move create database to procedure #3626

Merged

Conversation

CookiePieWw
Copy link
Contributor

@CookiePieWw CookiePieWw commented Apr 1, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

close #3591

What's changed and what's your intention?

move create database to procedure

Checklist

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

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Apr 1, 2024
@CookiePieWw
Copy link
Contributor Author

CookiePieWw commented Apr 1, 2024

The greptime-proto related part has not been finished yet, so I create a draft pr. I'll create another pr to greptime-proto once finished.

BTW, I found the CreateDatabaseExpr already defined in greptime-proto, which contains different params compared to the original impl of create database like the map option and missing catalog. I wonder how I would deal with it in the procedure.

updates:

  • I've found the option used in SchemaManager::create, so I think the procedure nees to take the option as a param as well. The option is empty by default
  • I added a catalog field to the definition of CreateDatabaseExpr

@WenyXu
Copy link
Member

WenyXu commented Apr 3, 2024

The greptime-proto related part has not been finished yet, so I create a draft pr. I'll create another pr to greptime-proto once finished.

BTW, I found the CreateDatabaseExpr already defined in greptime-proto, which contains different params compared to the original impl of create database like the map option and missing catalog. I wonder how I would deal with it in the procedure.

updates:

  • I've found the option used in SchemaManager::create, so I think the procedure nees to take the option as a param as well. The option is empty by default
  • I added a catalog field to the definition of CreateDatabaseExpr

If you don't mind, you can replace the greptime-proto version with your PR's commit hash.

@CookiePieWw
Copy link
Contributor Author

If you don't mind, you can replace the greptime-proto version with your PR's commit hash.

Got it. The pr is ready for review.

@CookiePieWw CookiePieWw marked this pull request as ready for review April 3, 2024 12:58
Copy link
Member

@WenyXu WenyXu left a comment

Choose a reason for hiding this comment

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

Nice job👍

src/common/meta/src/rpc/ddl.rs Outdated Show resolved Hide resolved
@CookiePieWw
Copy link
Contributor Author

I ran several tests since CI failed and I found the errors are mostly caused by case sensitivity and invalid schema names. But when I tried to reproduce and debug it on my mysql client, the results seem to be right. I'm a little confused. Maybe the client does something to make it right?

@WenyXu
Copy link
Member

WenyXu commented Apr 3, 2024

I ran several tests since CI failed and I found the errors are mostly caused by case sensitivity and invalid schema names. But when I tried to reproduce and debug it on my mysql client, the results seem to be right. I'm a little confused. Maybe the client does something to make it right?

All tests passed. BTW, would you like to add a fuzz test for database creation (in another PR).

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 84.82%. Comparing base (d33435f) to head (0040a15).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3626      +/-   ##
==========================================
- Coverage   85.16%   84.82%   -0.34%     
==========================================
  Files         943      944       +1     
  Lines      156940   157270     +330     
==========================================
- Hits       133661   133408     -253     
- Misses      23279    23862     +583     

@CookiePieWw
Copy link
Contributor Author

All tests passed. BTW, would like to add a fuzz test for database creation (in another PR).

Sure I'd like to.

@WenyXu
Copy link
Member

WenyXu commented Apr 3, 2024

All tests passed. BTW, would like to add a fuzz test for database creation (in another PR).

Sure I'd like to.

Have fun😙

Cargo.toml Outdated Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@waynexia waynexia mentioned this pull request Apr 7, 2024
5 tasks
@shuiyisong
Copy link
Contributor

shuiyisong commented Apr 7, 2024

Hi, nice work!
I've noticed that you used database_name in the proto definition, whereas we typically use schema_name (corresponding to catalog_name, as we do in other expression definitions). It would be great if you could modify the proto and this PR accordingly, so the code aligns with the others. 😃

@CookiePieWw
Copy link
Contributor Author

CookiePieWw commented Apr 7, 2024

Hi, nice work! I've noticed that you used database_name in the proto definition, whereas we typically use schema_name (corresponding to catalog_name, as we do in other expression definitions). It would be great if you could modify the proto and this PR accordingly, so the code aligns with the others. 😃

Since the pr of greptime-proto has been merged, it may need another update on the commit hash. Maybe we can leave it with some future clean-up work?

@MichaelScofield MichaelScofield added this pull request to the merge queue Apr 8, 2024
Merged via the queue into GreptimeTeam:main with commit c4798d1 Apr 8, 2024
18 checks passed
@CookiePieWw CookiePieWw deleted the move-createdb-to-proc branch April 8, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

move create database to procedure
5 participants