[I] make `ExecutionPlan` self contained. (allow for computed properties `PlanProperties`) [arrow-datafusion]

["universalmind303 (via GitHub)" <gi...@apache.org>]

universalmind303 opened a new issue, #10018:
URL: https://github.com/apache/arrow-datafusion/issues/10018

   ### Is your feature request related to a problem or challenge?
   
   
   if you have some structs that you want to loosely couple to datafusion, it is now impossible. 
   
   for example with <37
   
   
   ```rs
   #[derive(Debug, Clone)]
   pub struct MyStruct {
       projection: Arc<Schema>,
       predicate_projection: Arc<Schema>,
       // ...
   }
   ```
   
   and then you can just implement the trait as one would expect
   ```
   impl ExecutionPlan for MyStruct {
   
       fn output_partitioning(&self) -> Partitioning {
           Partitioning::UnknownPartitioning(1)
       }
   
       fn output_ordering(&self) -> Option<&[datafusion::physical_expr::PhysicalSortExpr]> {
           None
       }
    // ...   
   }
   
   ```
   
   But with datafusion 37, the datafusion specific components are now leaked into the outer struct, and force the user to modify the struct to contain `PlanProperties`. 
   
   
   ```rs
   impl ExecutionPlan for MyStruct {
     fn properties(&self) -> &PlanProperties {
       // no possible way to create `&PlanProperties`. 
       // the only option is to add it as a field on `MyStruct`
       // which breaks the encapsulation of `ExecutionPlan`
     }
   }
   ```
   
   
   ### Describe the solution you'd like
   
   modify `ExecutionPlan` to not have any methods that return a reference. Specifically `ExecutionPlan::properties`
   
   If a reference is wanted for performance reasons, we should instead use `Cow<PlanProperties>`.
   
   
   ### Describe alternatives you've considered
   
   NA
   
   ### Additional context
   
   _No response_


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

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

Re: [I] make `ExecutionPlan` self contained. (allow for computed properties `PlanProperties`) [arrow-datafusion]

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on issue #10018:
URL: https://github.com/apache/arrow-datafusion/issues/10018#issuecomment-2048373981

   Thank you for this idea @universalmind303 
   
   `Cow<PlanProperties>` seems like a good choice to me.
   
   Or else switching the trait back to only expose fields?
   
   We found it awkward to implement the new API in InfluxDB 3.0 as well (though we did make it work)
   
   Putting PlanProperties into the ExecutionPlan was to done to avoid recomputing the same (potentially expensive) properties over and over again as part of https://github.com/apache/arrow-datafusion/pull/9346
   
   


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

Re: [I] make `ExecutionPlan` self contained. (allow for computed properties `PlanProperties`) [arrow-datafusion]

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on issue #10018:
URL: https://github.com/apache/arrow-datafusion/issues/10018#issuecomment-2048368727

   cc @mustafasrepo  for your comments


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

Re: [I] make `ExecutionPlan` self contained. (allow for computed properties `PlanProperties`) [arrow-datafusion]

["mustafasrepo (via GitHub)" <gi...@apache.org>]

mustafasrepo commented on issue #10018:
URL: https://github.com/apache/arrow-datafusion/issues/10018#issuecomment-2055657487

   I think returning `Cow<PlanProperties>` makes sense. In this way, struct won't need to implement a new member as you indicated. We might need to update methods of the below
   ```
   impl ExecutionPlanProperties for Arc<dyn ExecutionPlan> {
       fn output_partitioning(&self) -> &Partitioning {
           self.properties().output_partitioning()
       }
   
       fn execution_mode(&self) -> ExecutionMode {
           self.properties().execution_mode()
       }
   
       fn output_ordering(&self) -> Option<&[PhysicalSortExpr]> {
           self.properties().output_ordering()
       }
   
       fn equivalence_properties(&self) -> &EquivalenceProperties {
           self.properties().equivalence_properties()
       }
   }
   ```
   to work with `Cow<PlanProperties>` though. 


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