-
Notifications
You must be signed in to change notification settings - Fork 868
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: add catalog/schema subcommands to flight_sql_client. #6332
Conversation
With this change basic commands are added to query the catalogs and schemas of a Flight SQL server.
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 @nathanielc 👋
This looks like a great contribution
Could you please also add tests for these new commands? I think you should be able to follow the existing pattern in
https://github.com/apache/arrow-rs/blob/master/arrow-flight/tests/flight_sql_client_cli.rs
cc @crepererum
Implementation looks good, but I agree with @alamb that a test is required. Doesn't have to be super sophisticated though, it's mostly a smoke test to check that the wiring is sound. |
@crepererum @alamb I have added tests, somehow I missed that they existed when I first looked. To make implementing the service easier, I copied the builder pattern from the CommandGetDbSchemas for the CommandGetTableTypes. While its a trivial implementation it makes implementing the service simple as it follows the same pattern as the other commands. |
7ce8b54
to
f8d60ee
Compare
Additionally adds a builder pattern for the CommandGetTableTypes similar to CommandGetDbSchemas, while its implementation is trivial it helps to have a pattern to follow when implementing the command.
f8d60ee
to
9c876b4
Compare
I've started the CI checks and clippy complaints about a (minor) issue, the overall approach/code looks good though. |
@crepererum Thanks I fixed the clippy issue. |
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 🙂
Which issue does this PR close?
Closes #6331
Rationale for this change
Adds the proposed solution from the issue.
What changes are included in this PR?
Are there any user-facing changes?
Yes, the new subcommands on the CLI are documented, producing good help text within the CLI.