houqp commented on a change in pull request #1013:
URL: https://github.com/apache/arrow-datafusion/pull/1013#discussion_r712360224
##########
File path: datafusion/src/execution/context.rs
##########
@@ -532,17 +533,27 @@ impl ExecutionContext {
}
/// Creates a physical plan from a logical plan.
- pub fn create_physical_plan(
+ pub async fn create_physical_plan(
&self,
logical_plan: &LogicalPlan,
) -> Result<Arc<dyn ExecutionPlan>> {
- let mut state = self.state.lock().unwrap();
- state.execution_props.start_execution();
+ let (state, planner) = {
+ let mut state = self.state.lock().unwrap();
+ state.execution_props.start_execution();
+
+ // We need to clone `state` to release the lock that is not
`Send`. We could
+ // make the lock `Send` by using `tokio::sync::Mutex`, but that
would require to
+ // propagate async even to the `LogicalPlan` building methods.
+ // Cloning `state` here is fine as we then pass it as immutable
`&state`, which
+ // means that we avoid write consistency issues as the cloned
version will not
+ // be written to. As for eventual modifications that would be
applied to the
+ // original state after it has been cloned, they will not be
picked up by the
+ // clone but that is okay, as it is equivalent to postponing the
state update
+ // by keeping the lock until the end of the function scope.
Review comment:
:+1: thanks for the clear write up
--
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]
