alamb commented on code in PR #8739:
URL: https://github.com/apache/arrow-datafusion/pull/8739#discussion_r1440911617
##########
datafusion-cli/src/exec.rs:
##########
@@ -165,9 +166,15 @@ pub async fn exec_from_repl(
}
Ok(line) => {
rl.add_history_entry(line.trim_end())?;
- match exec_and_print(ctx, print_options, line).await {
- Ok(_) => {}
- Err(err) => eprintln!("{err}"),
+ tokio::select! {
+ res = exec_and_print(ctx, print_options, line) => match
res {
Review Comment:
🤔 it might be good to add some documentation to the
https://docs.rs/datafusion/latest/datafusion/physical_plan/trait.ExecutionPlan.html#tymethod.execute
to explain the drop / cancel behavior and expectations this better. I will add
that to my list
##########
datafusion-cli/src/exec.rs:
##########
@@ -165,9 +166,15 @@ pub async fn exec_from_repl(
}
Ok(line) => {
rl.add_history_entry(line.trim_end())?;
- match exec_and_print(ctx, print_options, line).await {
- Ok(_) => {}
- Err(err) => eprintln!("{err}"),
+ tokio::select! {
+ res = exec_and_print(ctx, print_options, line) => match
res {
Review Comment:
I think this code is correct.
My understanding of what happens is that if the signal is handled, it will
`drop` the future running `exec_and_print` which will implicitly is `drop`ed
which should then propagate back and cancel outstanding tasks
There are a variety of tests that validate that outstanding tasks are
cancelled when their containing streams are dropped such as
https://github.com/apache/arrow-datafusion/blob/1179a76567892b259c88f08243ee01f05c4c3d5c/datafusion/physical-plan/src/coalesce_partitions.rs#L216-L234
If we find instances where dropping a stream doesn't cancel the ongoing
execution, I think we should file that as a bug and fix it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
