berkaysynnada commented on code in PR #15030:
URL: https://github.com/apache/datafusion/pull/15030#discussion_r1984885052
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -260,13 +260,30 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
/// used.
/// Thus, [`spawn`] is disallowed, and instead use [`SpawnedTask`].
///
+ /// To enable timely cancellation, the [`Stream`] that is returned must not
+ /// pin the CPU and must yield back to the tokio runtime regularly. This
can
Review Comment:
> > In the current architecture of datafusion, we are only introduced with
Pending results because of an IO work. So, tasks can only yield at IO points
(there are a few exceptions).
>
> This is not true, we already do cooperative yielding, see #5299 and the
bug report that was linked in there.
That is one of the exceptions I was referring to. However we know that is a
very specific edge case arising from the nature of current `RepartitionExec`.
Unless I am overlooking certain cases [this
version](https://github.com/apache/datafusion/pull/14411) should not cause any
problem within a scenario mentioned in the issue.
I stepped in here to prevent any misunderstanding that might lead people to
add manual yields at arbitrary points. While some well-reasoned designs serving
to specific use cases may require this, we should emphasize that it should be
done consciously. I understand that adding a few random yields may not have a
noticeable impact in practice, but this shouldn't spread throughout the
codebase.
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
