-
Notifications
You must be signed in to change notification settings - Fork 314
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
fix: cli export "create table" with quoted names #3684
fix: cli export "create table" with quoted names #3684
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3684 +/- ##
==========================================
+ Coverage 85.17% 85.20% +0.02%
==========================================
Files 955 957 +2
Lines 158994 159226 +232
==========================================
+ Hits 135431 135673 +242
+ Misses 23563 23553 -10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Good job! Could you add a test for it? |
@killme2008 First use I then switch to use greptimedb/tests-integration/src/standalone.rs Lines 42 to 49 in c00c1d9
greptime cli export with it. I think I need to craft Export first greptimedb/src/cmd/src/cli/export.rs Lines 118 to 125 in c00c1d9
export_create_table , but I don't make it.
|
@dimbtp I think you can refer to how the repl was tested before in "src/cmd/tests/cli.rs". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared concern.
@dimbtp I mean you can use the same methods like how to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this. But may other reviewers would suggest how to add tests further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@killme2008 @MichaelScofield @tisonkun A simple test and PTAL.
Signed-off-by: tison <wander4096@gmail.com>
@dimbtp the bin based test are super dirty. Since we already in the cmd crate, we can write more reliable and clean test cases. (I push a commit to refactor it.) @MichaelScofield perhaps the REPL test case can leverage the code I push in the last commit .. |
However, still thank a lot to nudge and drive this effort! |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
close #3681
What's changed and what's your intention?
Solve the problem that running
cli export
with--target create-table
fails when thecatalog/schema/table
names contain.
(e.g.a.b.c
).Checklist