ozankabak commented on code in PR #9346:
URL: https://github.com/apache/arrow-datafusion/pull/9346#discussion_r1505159403
##########
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;
/// Specifies how the output of this `ExecutionPlan` is split into
/// partitions.
- fn output_partitioning(&self) -> Partitioning;
+ fn output_partitioning(&self) -> &Partitioning {
Review Comment:
I think @mustafasrepo addressed all the reviews including naming, this is
the only one left. I don't think I fully understand your point, can you explain
a little more?
Specifically, the only difference I see with @mustafasrepo's design (i.e.
default implementations) and using a new extension trait is that the former
leaves the door open for a `ExecutionPlan` implementation to override the
simple "get the property from cache" behavior for any of the functions like
`output_partitioning`. I think this was by design, in case a downstream
requirement forces implementors to add some custom behavior there.
Do you think that may be undesirable (or have some possibly harmful results)?
--
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]
