alamb commented on PR #17675: URL: https://github.com/apache/datafusion/pull/17675#issuecomment-3343247962
> > I think the changes to avoid Schema clones will help , though I just double checked and a Schema is mostly an empty HashMap and an Arc (Fields) so clone'ing isn't that expensive. > > That's a good point, I didn't look into it. I'm open to reverting the changes in the logical plan builder from `SchemaRef` back to `&Schema` to reduce breaking changes but I wouldn't change `logical2physical`. > > > Also, I didn't find many places in the code by inspection where we saved a deep clone of a LogicalPlan -- did I miss any that you know of? > > I don't think there's any deep cloning - after all the `LogicalPlan` children are also `Arc`s. But I also don't see much of a downside if we use `impl Into<Arc<LogicalPlan>>` since it allows using either one. I agree this change seems non disruptive for downstream users and is a good plan > > > If we are going to change the APIs around and disrupt downstream users, I think they should get something from it, in this case better planning performance. > > I guess that really depends on how much we use the logical plan builder internally. I think it is used frequently for planning and Dataframe APIs: https://github.com/apache/datafusion/blob/62e6d5e259d32f590b6d61bad6b08ece7b3416b9/datafusion/core/src/dataframe/mod.rs#L392-L391 -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
