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

fix: cli export "create table" with quoted names #3684

Conversation

dimbtp
Copy link
Contributor

@dimbtp dimbtp commented Apr 9, 2024

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 the catalog/schema/table names contain . (e.g. a.b.c).

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 9, 2024
Copy link

codecov bot commented Apr 9, 2024

Codecov Report

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

Project coverage is 85.20%. Comparing base (aab7367) to head (c3b4fc4).
Report is 3 commits behind head on main.

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     

Copy link
Collaborator

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you!

@killme2008
Copy link
Contributor

Good job! Could you add a test for it?

@dimbtp
Copy link
Contributor Author

dimbtp commented Apr 10, 2024

Good job! Could you add a test for it?

@killme2008
I did some investigations and tried following 2 approaches:

First use std::process::Command::new to spawn two threads, one is standalone server and another is cli export to test the function.
Then I plan to create target database and table with quoted name (e.g. test-export.a.b.c), in locally I manually add it with mysql repl, but whether it's OK to do that in spawning a mysql thread?

I then switch to use GreptimeDbStandalone

pub struct GreptimeDbStandalone {
pub instance: Arc<Instance>,
pub mix_options: MixOptions,
pub guard: TestGuard,
// Used in rebuild.
pub kv_backend: KvBackendRef,
pub procedure_manager: ProcedureManagerRef,
}
, which can easily run sql but I don't know how to communicate greptime cli export with it. I think I need to craft Export first
pub struct Export {
client: Database,
catalog: String,
schema: Option<String>,
output_dir: String,
parallelism: usize,
target: ExportTarget,
}
then call export_create_table, but I don't make it.

@MichaelScofield
Copy link
Collaborator

@dimbtp I think you can refer to how the repl was tested before in "src/cmd/tests/cli.rs".

Copy link
Collaborator

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Shared concern.

src/cmd/src/cli/export.rs Outdated Show resolved Hide resolved
@MichaelScofield
Copy link
Collaborator

@dimbtp I mean you can use the same methods like how to use Command or how to handle the client in src/cmd/tests/cli.rs.

Copy link
Collaborator

@tisonkun tisonkun left a 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.

Copy link
Contributor Author

@dimbtp dimbtp left a 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.

src/cmd/src/cli/export.rs Outdated Show resolved Hide resolved
src/cmd/src/cli/export.rs Outdated Show resolved Hide resolved
src/cmd/src/cli/export.rs Outdated Show resolved Hide resolved
src/cmd/src/cli/export.rs Outdated Show resolved Hide resolved
@dimbtp
Copy link
Contributor Author

dimbtp commented Apr 10, 2024

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Collaborator

tisonkun commented Apr 10, 2024

@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 ..

@tisonkun
Copy link
Collaborator

tisonkun commented Apr 10, 2024

However, still thank a lot to nudge and drive this effort!

@MichaelScofield MichaelScofield added this pull request to the merge queue Apr 11, 2024
Merged via the queue into GreptimeTeam:main with commit 02f806f Apr 11, 2024
19 checks passed
@dimbtp dimbtp deleted the fix/cli-export-create-table-with-quote branch April 16, 2024 03:06
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.

cli export "create table" failed
4 participants