Add statement helper command to cli #1285
Conversation
|
@jimexist this is rough around the edges right now, but i added two commands: |
datafusion-cli/src/command.rs
Outdated
| @@ -34,6 +35,8 @@ pub enum Command { | |||
| Help, | |||
| ListTables, | |||
| DescribeTable(String), | |||
| FunctionList, | |||
There was a problem hiding this comment.
how about using ListFunctions to keep it consistent?
There was a problem hiding this comment.
yup, was going to update that
|
@jimexist is it ok to use the same definitions as psql (only including relevant parts of course)? |
yes please - I think to reduce user memory overhead is the goal here, unless there's a good reason to break that consistency |
There was a problem hiding this comment.
Thanks @matthewmturner. Left two comments. Others look good to me.
datafusion-cli/src/command.rs
Outdated
| @@ -64,6 +67,11 @@ impl Command { | |||
| Self::Quit => Err(DataFusionError::Execution( | |||
| "Unexpected quit, this should be handled outside".into(), | |||
| )), | |||
| Self::ListFunctions => display_all_functions(), | |||
| Self::SearchFunctions(function) => { | |||
| let func = function.parse::<Function>().unwrap(); | |||
There was a problem hiding this comment.
It seems better to return an err message when the parse fails instead of crashing the program.
datafusion-cli/src/functions.rs
Outdated
| type Err = (); | ||
|
|
||
| fn from_str(s: &str) -> std::result::Result<Self, Self::Err> { | ||
| Ok(match s.to_uppercase().as_str() { |
There was a problem hiding this comment.
The cli crashes when function name surrounded whitespace so how about:
| Ok(match s.to_uppercase().as_str() { | |
| Ok(match s.trim().to_uppercase().as_str() { |
@capkurmagati thanks for feedback - agree on those points. ive updated. |
|
@jimexist ready when you get the chance |
datafusion-cli/src/command.rs
Outdated
| if let Ok(func) = function.parse::<Function>() { | ||
| func.function_details()? | ||
| } else { | ||
| eprintln!("{} is not a supported function", function) |
There was a problem hiding this comment.
how about you just return error for this case?
datafusion-cli/src/functions.rs
Outdated
| pub fn function_details(&self) -> Result<()> { | ||
| match self { | ||
| Function::Select => { | ||
| let details = " |
There was a problem hiding this comment.
you can use r#" "# to mark multiline raw string
datafusion-cli/src/functions.rs
Outdated
| Syntax: | ||
| EXPLAIN [ ANALYZE ] statement | ||
| "; | ||
| println!("{}", details) |
There was a problem hiding this comment.
how about returning the detail and print outside the match?
|
@jimexist thx for review. i believe ive addressed the comments. |
Co-authored-by: Jiayu Liu <Jimexist@users.noreply.github.com>
|
Thanks @matthewmturner ! |
Which issue does this PR close?
Closes #1227
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?