gruuya commented on code in PR #9346:
URL: https://github.com/apache/arrow-datafusion/pull/9346#discussion_r1503865191
##########
datafusion/physical-plan/src/lib.rs:
##########
@@ -121,21 +121,23 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
fn as_any(&self) -> &dyn Any;
/// Get the schema for this execution plan
- fn schema(&self) -> SchemaRef;
+ fn schema(&self) -> SchemaRef {
+ self.cache().schema().clone()
+ }
+
+ fn cache(&self) -> &PlanPropertiesCache;
Review Comment:
First of all thanks for this great comprehensive solution and the effort
here, very happy to see this! I think that this change could have some
interesting performance consequences for the better.
I just have a small nit: I feel like the plain name `cache` here could be
confusing, especially in the context of an `ExecutionPlan`, as it implies cache
of data/batches. While it's more clear from the signature, in other places
throughout the code it may be less so.
Some potential candidates that sound more descriptive to me are
`plan_props`, `props_cache`, `properties` and other such combinations.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
