Propagate error from spawned task reading spills #7510
Conversation
| Ok(_) => continue, | ||
| // This means a blocking task error | ||
| Err(e) => { | ||
| return Some(internal_err!("Spawned Task error: {e}")); |
There was a problem hiding this comment.
For example, after this, if the file doesn't exist, you will get an error:
Error: Internal("Spawned Task error: Execution error: Spill file \"/var/folders/zq/2tdnn5955wvdcw7qfy6qhvk00000gn/T/.tmpb5EGzS/.tmptpkruZ\" does not exist")
There was a problem hiding this comment.
ExecutionError might be more appropriate than InternalError given an error in a spill file is likely not a bug in DataFusion, but rather some issue in the environment.
|
|
||
| builder.spawn(async move { | ||
| while let Some(item) = input.next().await { | ||
| if sender.send(item).await.is_err() { |
There was a problem hiding this comment.
The only reason an item can't be sent via a channel I think is if the other end (the Receiver) was dropped. This can certainly happen if some other part of the query errors, but I also think it can happen with the plan shuts down early due to a LIMIT or something similar.
Thus I am not sure we should propagate an error in this case
There was a problem hiding this comment.
Makes sense. Removed the error.
| Ok(_) => continue, | ||
| // This means a blocking task error | ||
| Err(e) => { | ||
| return Some(internal_err!("Spawned Task error: {e}")); |
There was a problem hiding this comment.
ExecutionError might be more appropriate than InternalError given an error in a spill file is likely not a bug in DataFusion, but rather some issue in the environment.
|
Thank you @alamb |
2 participants
Which issue does this PR close?
Closes #7509.
Rationale for this change
Any error happened in the spawned task reading spills cannot propagate out currently. So these errors will be ignored and it doesn't look correct as the sorted result will be incorrect.
What changes are included in this PR?
Propagating errors from spawned task reading spills.
Are these changes tested?
Are there any user-facing changes?