[GitHub] [arrow-datafusion] tustvold commented on a diff in pull request #2713: feat: async extension planner

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

tustvold commented on code in PR #2713:
URL: https://github.com/apache/arrow-datafusion/pull/2713#discussion_r894287800


##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -292,10 +293,10 @@ pub trait ExtensionPlanner {
     /// Returns `None` when the planner does not know how to plan the
     /// `node` and wants to delegate the planning to another
     /// [`ExtensionPlanner`].
-    fn plan_extension(
+    async fn plan_extension(
         &self,
-        planner: &dyn PhysicalPlanner,
-        node: &dyn UserDefinedLogicalNode,
+        planner: &(dyn PhysicalPlanner + Send + Sync),
+        node: Arc<dyn UserDefinedLogicalNode + Send + Sync>,

Review Comment:
   Why is the change to `Arc` needed?



##########
datafusion/core/src/physical_plan/planner.rs:
##########
@@ -292,10 +293,10 @@ pub trait ExtensionPlanner {
     /// Returns `None` when the planner does not know how to plan the
     /// `node` and wants to delegate the planning to another
     /// [`ExtensionPlanner`].
-    fn plan_extension(
+    async fn plan_extension(
         &self,
-        planner: &dyn PhysicalPlanner,
-        node: &dyn UserDefinedLogicalNode,
+        planner: &(dyn PhysicalPlanner + Send + Sync),

Review Comment:
   I wonder if the `Send` and `Sync` constraints should be added to `PhysicalPlanner` and `UserDefinedLogicalNode` instead of at this specific call-site?



-- 
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-unsubscribe@arrow.apache.org

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