adriangb commented on code in PR #20009:
URL: https://github.com/apache/datafusion/pull/20009#discussion_r2769241999
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -722,6 +722,51 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
) -> Option<Arc<dyn ExecutionPlan>> {
None
}
+
+ /// Returns an iterator over a subset of [`PhysicalExpr`]s that are used
by this
+ /// [`ExecutionPlan`].
+ ///
+ /// This method provides a way to inspect the physical expressions within
a node without
+ /// knowing its specific type. These expressions could be predicates in a
filter, projection
+ /// expressions, or join conditions, etc.
+ ///
+ /// The default implementation returns `None`, indicating that the node
either has no physical
+ /// expressions or does not support exposing them.
+ fn physical_expressions<'a>(
+ &'a self,
+ ) -> Option<Box<dyn Iterator<Item = Arc<dyn PhysicalExpr>> + 'a>> {
+ None
Review Comment:
Same note about removing default impl in the future.
##########
datafusion/core/tests/physical_optimizer/pushdown_utils.rs:
##########
@@ -155,6 +155,16 @@ impl FileSource for TestSource {
})
}
+ fn with_filter_and_projection(
Review Comment:
This is not mentioned at all in the PR description. Can you please explain
why this is needed?
##########
datafusion/datasource/src/source.rs:
##########
@@ -215,6 +215,39 @@ pub trait DataSource: Send + Sync + Debug {
fn with_preserve_order(&self, _preserve_order: bool) -> Option<Arc<dyn
DataSource>> {
None
}
+
+ /// Returns an iterator over a subset of [`PhysicalExpr`]s that are used
by this [`DataSource`].
+ fn physical_expressions<'a>(
+ &'a self,
+ ) -> Option<Box<dyn Iterator<Item = Arc<dyn PhysicalExpr>> + 'a>> {
+ None
Review Comment:
I think it would make sense to in some future version require every
`ExecutionPlan` to implement this even if it's a no op. Otherwise it's easy to
forget. I think enough `ExecutionPlan`s would have expressions that it's worth
forcing implementers to make a decision.
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -722,6 +722,51 @@ pub trait ExecutionPlan: Debug + DisplayAs + Send + Sync {
) -> Option<Arc<dyn ExecutionPlan>> {
None
}
+
+ /// Returns an iterator over a subset of [`PhysicalExpr`]s that are used
by this
+ /// [`ExecutionPlan`].
+ ///
+ /// This method provides a way to inspect the physical expressions within
a node without
+ /// knowing its specific type. These expressions could be predicates in a
filter, projection
+ /// expressions, or join conditions, etc.
+ ///
+ /// The default implementation returns `None`, indicating that the node
either has no physical
+ /// expressions or does not support exposing them.
+ fn physical_expressions<'a>(
Review Comment:
cc @gabotechs we were discussing adding just this the other day
##########
datafusion/datasource/src/file.rs:
##########
@@ -78,6 +78,14 @@ pub trait FileSource: Send + Sync {
fn projection(&self) -> Option<&ProjectionExprs> {
None
}
+ /// Returns new file source with given filter and projection.
+ fn with_filter_and_projection(
Review Comment:
If we do add this I think it's worth making it *very clear* how this is
intended to be used.
For example, users should *not* call this from their custom `TableProvider`
implementation to pass in projections / filters. That pushdown is handled by
optimizer rules. This is only to be used by ...
--
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]
