Change cli to propagate error to exit code

[tshauck]

Which issue does this PR close?

TODO:

Rationale for this change

The rationale is that currently when using the cli, if you use -c and -f, both of which are useful to use datafusion in workflows (e.g. and ETL pipeline), the exit code if the query fails is still 0. Most job orchestration tools can propagate the exit code from a CLI tool "up", so this makes using the CLI more ergonomic for that use case.

What changes are included in this PR?

Generally, adds Result return enums so that the CLI will eventually return the 0 or 1 exit code to the caller.

Are these changes tested?

I ran the -c, -f, and interactive mode. Things are functional, though the main difference I've noticed is the error printed out is a bit more verbose right now. E.g.

./target/debug/datafusion-cli -c 'SELECT hi';        
DataFusion CLI v34.0.0
Error: SchemaError(FieldNotFound { field: Column { relation: None, name: "hi" }, valid_fields: [] }, Some(""))

echo $?;
1

If this is to be merged, I'll try to cleanup the error message, but I thought I'd open this to get feedback as to if this is something the project would consider merging.

Thanks

[alamb]

This makes a lot of sense to me @tshauck -- thank you 🙏

[tshauck]

Thanks @alamb... so looking into the error display, it looks like the printed message is Debug. So certainly, open to alternatives, but what I did to start is implement a new DataFusionCLIError where Debug and Display just show an inner message. It also implements From for datafusion error and rustyline error so they can be transformed into the new error.

With those changes, this is what the error now looks like:

./target/debug/datafusion-cli -c 'SELECT hi;'               
DataFusion CLI v34.0.0
Error: Schema error: No field named hi.

[alamb]

Thanks @tshauck

[alamb]

Rather than implementing an entire new error class just to reuse the existing error handler, what do you think about handling the error explicitly? Perhaps something like

#[tokio::main]
/// Handle errors in `main_inner`
pub async fn main() {
 // print nice error and return error status from process on error
  if let Err(e) = main_inner().await {
    println!("Execution Error: {e}");
    std::process:exit(1);
  }
}

/// Main CLI entrypoint
async fn main_inner() -> Result<()> {
    env_logger::init();
    let args = Args::parse();
...
}

[tshauck]

Works for me too... I'll take another crack at it this evening and follow up when it's ready.

[tshauck]

I ended up going a slightly different route and wrapped main as suggested but am having it return ExitCode vs ().

From the std::process::exit docs, it seems like while it can work, it's preferable to either return Result (initial impl if verbose) or ExitCode (i.e. something that implements Termination) to better handle cleanup, not that it matters a ton here, but 🤷 .

Note that because this function never returns, and that it terminates the process, no destructors on the current stack or any other thread’s stack will be run. If a clean shutdown is needed it is recommended to only call this function at a known point where there are no more destructors left to run; or, preferably, simply return a type implementing Termination (such as ExitCode or Result) from the main function and avoid this function altogether:

https://doc.rust-lang.org/std/process/fn.exit.html

Happy to go w/ exit if you still think it's the best route.

[alamb]

Looks good to me -- thank you @tshauck

[alamb]

👍

[alamb]

I also double checked with a quick test locally:

venv) andrewlamb@Andrews-MacBook-Pro:~/Software/arrow-datafusion/datafusion-cli$ ./target/debug/datafusion-cli  -c 'select foo()' || echo "fail"
DataFusion CLI v34.0.0
Error: Error during planning: Invalid function 'foo'.
Did you mean 'cos'?
fail
(venv) andrewlamb@Andrews-MacBook-Pro:~/Software/arrow-datafusion/datafusion-cli$ ./target/debug/datafusion-cli  -c 'select 1' || echo "fail"
DataFusion CLI v34.0.0
+----------+
| Int64(1) |
+----------+
| 1        |
+----------+
1 row in set. Query took 0.014 seconds.

And it seems to work great. Thanks @tshauck !