[GitHub] [arrow] andygrove commented on a change in pull request #8079: ARROW-9815: [Rust][DataFusion] Remove the use of Arc/Mutex to protect plan time structures

[GitBox <gi...@apache.org>]

andygrove commented on a change in pull request #8079:
URL: https://github.com/apache/arrow/pull/8079#discussion_r479790490



##########
File path: rust/datafusion/src/execution/context.rs
##########
@@ -255,26 +253,17 @@ impl ExecutionContext {
 
     /// Register a table using a custom TableProvider so that it can be referenced from SQL
     /// statements executed against this context.
-    pub fn register_table(
-        &mut self,
-        name: &str,
-        provider: Box<dyn TableProvider + Send + Sync>,
-    ) {
-        let mut ctx_state = self.state.lock().expect("failed to lock mutex");
-        ctx_state.datasources.insert(name.to_string(), provider);
+    pub fn register_table(&mut self, name: &str, provider: Box<dyn TableProvider>) {

Review comment:
       I agree that `TableProvider` no longer needs `Send` and `Sync` now that it is just used during planning and no longer performs execution.
   
   I don't think changing this will affect whether ExecutionContext can be sent between threads but I don't have time today to actually confirm that.
   
   If we do want to be sure that `ExecutionContext` can be shared between threads, we should add a unit test for that.
   
   Hopefully, I'll be able to find some time this evening to look at this some more.
   
   




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org