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: PREPARE and EXECUTE statement from mysql client #4125

Merged

Conversation

CookiePieWw
Copy link
Collaborator

@CookiePieWw CookiePieWw commented Jun 9, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

close #769

What's changed and what's your intention?

As title.

Checklist

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

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label Jun 9, 2024
@CookiePieWw CookiePieWw marked this pull request as ready for review June 14, 2024 06:45
@CookiePieWw CookiePieWw requested a review from a team as a code owner June 14, 2024 06:45
@CookiePieWw
Copy link
Collaborator Author

Now it works on some simple cases I tried. For PREPARE stmt and on_prepare I tried to extract common logic into do_prepare. It's quite similar in EXECUTE stmt and on_execute, but I find it a little tricky for the writer and different parameter types (ParamParser for on_execute and Expr for the stmt). Maybe I could refactor it later.

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 138 lines in your changes missing coverage. Please review.

Project coverage is 84.82%. Comparing base (cc2f7ef) to head (5e33b1f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4125      +/-   ##
==========================================
- Coverage   85.12%   84.82%   -0.31%     
==========================================
  Files        1020     1023       +3     
  Lines      179635   179907     +272     
==========================================
- Hits       152920   152610     -310     
- Misses      26715    27297     +582     

@killme2008
Copy link
Contributor

killme2008 commented Jun 14, 2024

Now it works on some simple cases I tried. For PREPARE stmt and on_prepare I tried to extract common logic into do_prepare. It's quite similar in EXECUTE stmt and on_execute, but I find it a little tricky for the writer and different parameter types (ParamParser for on_execute and Expr for the stmt). Maybe I could refactor it later.

Cool, let's take a look! Thank you. @MichaelScofield @fengjiachun

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

Leave some suggestions and questions, please take a look.

I found that there are not enough tests currently, especially the unit tests, could you add some?

Thank you, it looks great!

src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
src/servers/src/mysql/helper.rs Show resolved Hide resolved
src/sql/src/parser.rs Show resolved Hide resolved
src/sql/src/parser.rs Outdated Show resolved Hide resolved
src/sql/src/parser.rs Show resolved Hide resolved
@CookiePieWw
Copy link
Collaborator Author

Leave some suggestions and questions, please take a look.

Got it. I'll make some changes later.

I found that there are not enough tests currently, especially the unit tests, could you add some?

Sure. I'll try to add some sqlness tests as well, maybe copy some from duckdb :)

@CookiePieWw CookiePieWw force-pushed the prepare-execute-stmt-from-cli branch from e6904c5 to 0b72305 Compare June 16, 2024 17:04
@CookiePieWw
Copy link
Collaborator Author

Changes have been done. The stmt_key is now an uuid string generated from stmt_id, so the StmtKey enum has been removed.

Since this only works for mysql client, I didn't find a way to add sqlness test for it. Maybe we can only test it manually.

Fuzz test on inserting logical table failed on some reason. Don't know if it's related to this pr. The log is similar to #4122:

2024-06-15T19:02:41.7471713Z 1: Database(MySqlDatabaseError { code: Some("HY000"), number: 1815, message: "Region 4432406249472(1032, 0) is in ReadOnly state, expect: Writable" })

src/servers/src/mysql/handler.rs Show resolved Hide resolved
src/servers/src/mysql/handler.rs Show resolved Hide resolved
src/sql/src/parser.rs Outdated Show resolved Hide resolved
@killme2008
Copy link
Contributor

Since this only works for mysql client, I didn't find a way to add sqlness test for it. Maybe we can only test it manually.

We can leave it in the next PR. Let's figure out how to test it in sqlness. Currently you can test it at

pub async fn test_mysql_crud(store_type: StorageType) {

Fuzz test on inserting logical table failed on some reason. Don't know if it's related to this pr. The log is similar to #4122:

2024-06-15T19:02:41.7471713Z 1: Database(MySqlDatabaseError { code: Some("HY000"), number: 1815, message: "Region 4432406249472(1032, 0) is in ReadOnly state, expect: Writable" })

Could you rebase the branch with the main? Looks like it's fixed already.

@killme2008
Copy link
Contributor

It's almost done, thank you so much! @CookiePieWw

Please take a look @MichaelScofield

src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
src/servers/src/mysql/handler.rs Show resolved Hide resolved
src/servers/src/mysql/handler.rs Show resolved Hide resolved
src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
src/servers/src/mysql/handler.rs Outdated Show resolved Hide resolved
@CookiePieWw
Copy link
Collaborator Author

Updated and added DEALLOCATE stmt. PTAL @killme2008 @MichaelScofield

Copy link
Contributor

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

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

Great work! Thanks a lot.

@MichaelScofield MichaelScofield added this pull request to the merge queue Jun 21, 2024
Merged via the queue into GreptimeTeam:main with commit b739c9f Jun 21, 2024
51 checks passed
@killme2008
Copy link
Contributor

@CookiePieWw Appreciate your contribution, thanks a lot.

@CookiePieWw CookiePieWw deleted the prepare-execute-stmt-from-cli branch June 21, 2024 04:41
@tisonkun
Copy link
Collaborator

@CookiePieWw Thanks for your contribution! Would you share your email address to me (by email to [email protected])?

I'd like to send you a letter for further engagement :D

zyy17 pushed a commit to zyy17/greptimedb that referenced this pull request Jun 22, 2024
)

* feat: prepare stmt in mysql client

* feat: execute stmt in mysql client

* fix: handle parameters properly

* refactor: use existing funcs to convert expr to scalar value

* refactor: use uuid strings as stmt_key for queries from COM_PREPARE packet

* refactor: take prepare and execute parser as submodule

* test: add unit test for converting expr to scalar value

* feat: deallocate stmt in mysql client

* chore: comments and duplicates

---------

Co-authored-by: dennis zhuang <[email protected]>
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.

Supports prepare and execute statement
4 participants