You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jayshrivastava (via GitHub)" <gi...@apache.org> on 2026/07/30 19:57:44 UTC
[PR] Js/dynamic filters method [datafusion]
jayshrivastava opened a new pull request, #24018:
URL: https://github.com/apache/datafusion/pull/24018
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #24018:
URL: https://github.com/apache/datafusion/pull/24018
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3729182855
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
Sounds good. I'll resolve this.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3738721322
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -247,6 +250,62 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync {
/// joins).
fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>>;
+ /// Apply a closure `f` to each expression in the current physical plan node. `f`
+ /// should not be called in any child expressions nor in any expressions of child nodes.
+ ///
+ /// The closure can return [`TreeNodeRecursion::Stop`] to stop iteration, otherwise
+ /// iteration should continue. ([`TreeNodeRecursion::Jump`] and [`TreeNodeRecursion::Jump`]
Review Comment:
I think this means to say "continue" and "jump" are equivalent. I will push a fix for this
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
it is done at line 315 it seems π€
##########
datafusion/datasource/src/source.rs:
##########
@@ -225,6 +226,22 @@ pub trait DataSource: Any + Send + Sync + Debug {
None
}
+ /// Apply a closure to each expression used by this data source.
+ ///
+ /// This includes filter predicates (which may contain dynamic filters) and any
+ /// other expressions used during data scanning.
+ ///
+ /// The function `f` should be called once per expression unless the function returns
+ /// [`TreeNodeRecursion::Stop`] to stop iteration.
+ ///
+ /// See [`ExecutionPlan::apply_expressions`] for more details and implementation examples.
+ ///
+ /// [`ExecutionPlan::apply_expressions`]: datafusion_physical_plan::ExecutionPlan::apply_expressions
Review Comment:
This is good to direct readers to ExecutionPlan::apply_expressions and then document that heavily
##########
datafusion/ffi/src/physical_expr/mod.rs:
##########
@@ -434,6 +436,7 @@ unsafe extern "C" fn clone_fn_wrapper(expr: &FFI_PhysicalExpr) -> FFI_PhysicalEx
snapshot: snapshot_fn_wrapper,
snapshot_generation: snapshot_generation_fn_wrapper,
is_volatile_node: is_volatile_node_fn_wrapper,
+ expression_id: expression_id_fn_wrapper,
Review Comment:
this appears to be an unrelated change, but a good one
##########
docs/source/library-user-guide/upgrading/55.0.0.md:
##########
@@ -986,205 +894,58 @@ let plan = deserialize_bytes(&proto_bytes)?;
See [PR #23827](https://github.com/apache/datafusion/pull/23827) for details.
-### `WindowExpr::evaluate_stateful` now takes a `WindowEvalContext`
-
-`WindowExpr::evaluate_stateful` (and the provided
-`AggregateWindowExpr::aggregate_evaluate_stateful` method) take a new
-`WindowEvalContext` argument carrying stream-level information that is shared
-by all partitions:
-
-```rust,ignore
-// Before
-fn evaluate_stateful(
- &self,
- partition_batches: &PartitionBatches,
- window_agg_state: &mut PartitionWindowAggStates,
-) -> Result<()>
-
-// After
-fn evaluate_stateful(
- &self,
- partition_batches: &PartitionBatches,
- window_agg_state: &mut PartitionWindowAggStates,
- eval_ctx: &WindowEvalContext<'_>,
-) -> Result<()>
-```
-
-`WindowEvalContext` currently carries the most recent input row, which
-previously lived in each partition's `PartitionBatchState` (see the next
-section). The struct is `#[non_exhaustive]` so that fields can be added
-without further signature changes: construct it with
-`WindowEvalContext::default()` and set fields through its builder methods.
-
-**Who is affected:**
-
-- Implementations of the `WindowExpr` trait that override `evaluate_stateful`
- must add the new parameter.
-- Callers of `evaluate_stateful` or `aggregate_evaluate_stateful` must pass a
- context.
-
-**Migration guide:**
-
-```rust,ignore
-use datafusion_physical_expr::window::WindowEvalContext;
-
-// Before
-window_expr.evaluate_stateful(&partition_batches, &mut window_agg_state)?;
-
-// After
-let eval_ctx = WindowEvalContext::default()
- .with_most_recent_row(most_recent_row.as_ref());
-window_expr.evaluate_stateful(
- &partition_batches,
- &mut window_agg_state,
- &eval_ctx,
-)?;
-```
-
-Pass `WindowEvalContext::default()` when no most-recent-row watermark is
-available (for example, when the input is sorted by the partition keys and
-partition ends are detected directly).
-
-### `PartitionBatchState::most_recent_row` removed
+### `ExecutionPlan::apply_expressions` is now a required method
-The `most_recent_row` field and the `set_most_recent_row` method have been
-removed from `datafusion_expr::window_state::PartitionBatchState`. The most
-recent input row is a property of the whole input stream rather than
-per-partition state: every partition observed the same value. It is now
-tracked once by the operator driving the evaluation and passed to window
-expressions through the new `WindowEvalContext` argument of
-`WindowExpr::evaluate_stateful` described above.
+`apply_expressions` has been added as a **required** method on the `ExecutionPlan`, `FileSource`, and `DataSource` traits. Any custom implementation of
+these traits must now implement `apply_expressions`.
**Who is affected:**
-- Code that read `PartitionBatchState::most_recent_row` or called
- `set_most_recent_row`, such as custom streaming window operators.
+- Users who implement custom `ExecutionPlan` nodes
+- Users who implement custom `FileSource` or `DataSource` sources
**Migration guide:**
-Track the most recent input row once per stream (for example, a one-row
-slice of the last non-empty input batch) and pass it to window expressions
-via `WindowEvalContext::with_most_recent_row` instead of copying it into
-each partition's state.
+Add `apply_expressions` to your implementation. Call `f` on each top-level
Review Comment:
Specifically the examples are redundant I think
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -852,6 +911,65 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync {
}
}
+/// Implements [`ExecutionPlan::apply_expressions`] for a node with no expressions.
+pub fn apply_no_expressions(
Review Comment:
I would suggest you remove this function (as it is pretty clearly documented what to do when there are no expressions, so it is just one more function that adds complexity but not much help I don't think).
I do realize there is a nice symmetry to have a helper for each case, but in this case I am not sure it is needed
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3738643098
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
How about this?
```rust
properties.apply_expressions(f)
```
##########
docs/source/library-user-guide/upgrading/55.0.0.md:
##########
@@ -986,205 +894,58 @@ let plan = deserialize_bytes(&proto_bytes)?;
See [PR #23827](https://github.com/apache/datafusion/pull/23827) for details.
-### `WindowExpr::evaluate_stateful` now takes a `WindowEvalContext`
-
-`WindowExpr::evaluate_stateful` (and the provided
-`AggregateWindowExpr::aggregate_evaluate_stateful` method) take a new
-`WindowEvalContext` argument carrying stream-level information that is shared
-by all partitions:
-
-```rust,ignore
-// Before
-fn evaluate_stateful(
- &self,
- partition_batches: &PartitionBatches,
- window_agg_state: &mut PartitionWindowAggStates,
-) -> Result<()>
-
-// After
-fn evaluate_stateful(
- &self,
- partition_batches: &PartitionBatches,
- window_agg_state: &mut PartitionWindowAggStates,
- eval_ctx: &WindowEvalContext<'_>,
-) -> Result<()>
-```
-
-`WindowEvalContext` currently carries the most recent input row, which
-previously lived in each partition's `PartitionBatchState` (see the next
-section). The struct is `#[non_exhaustive]` so that fields can be added
-without further signature changes: construct it with
-`WindowEvalContext::default()` and set fields through its builder methods.
-
-**Who is affected:**
-
-- Implementations of the `WindowExpr` trait that override `evaluate_stateful`
- must add the new parameter.
-- Callers of `evaluate_stateful` or `aggregate_evaluate_stateful` must pass a
- context.
-
-**Migration guide:**
-
-```rust,ignore
-use datafusion_physical_expr::window::WindowEvalContext;
-
-// Before
-window_expr.evaluate_stateful(&partition_batches, &mut window_agg_state)?;
-
-// After
-let eval_ctx = WindowEvalContext::default()
- .with_most_recent_row(most_recent_row.as_ref());
-window_expr.evaluate_stateful(
- &partition_batches,
- &mut window_agg_state,
- &eval_ctx,
-)?;
-```
-
-Pass `WindowEvalContext::default()` when no most-recent-row watermark is
-available (for example, when the input is sorted by the partition keys and
-partition ends are detected directly).
-
-### `PartitionBatchState::most_recent_row` removed
+### `ExecutionPlan::apply_expressions` is now a required method
-The `most_recent_row` field and the `set_most_recent_row` method have been
-removed from `datafusion_expr::window_state::PartitionBatchState`. The most
-recent input row is a property of the whole input stream rather than
-per-partition state: every partition observed the same value. It is now
-tracked once by the operator driving the evaluation and passed to window
-expressions through the new `WindowEvalContext` argument of
-`WindowExpr::evaluate_stateful` described above.
+`apply_expressions` has been added as a **required** method on the `ExecutionPlan`, `FileSource`, and `DataSource` traits. Any custom implementation of
+these traits must now implement `apply_expressions`.
**Who is affected:**
-- Code that read `PartitionBatchState::most_recent_row` or called
- `set_most_recent_row`, such as custom streaming window operators.
+- Users who implement custom `ExecutionPlan` nodes
+- Users who implement custom `FileSource` or `DataSource` sources
**Migration guide:**
-Track the most recent input row once per stream (for example, a one-row
-slice of the last non-empty input batch) and pass it to window expressions
-via `WindowEvalContext::with_most_recent_row` instead of copying it into
-each partition's state.
+Add `apply_expressions` to your implementation. Call `f` on each top-level
Review Comment:
This is fine, though it is technically redundant with the (very nice docs) on apply_expressions
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
(can be a follow on PR)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3742084388
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -852,6 +911,65 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync {
}
}
+/// Implements [`ExecutionPlan::apply_expressions`] for a node with no expressions.
+pub fn apply_no_expressions(
Review Comment:
Done
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3721873258
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -2097,27 +2151,12 @@ impl ExecutionPlan for AggregateExec {
if phase == FilterPushdownPhase::Post
&& let Some(dyn_filter) = &self.dynamic_filter
{
- // let child_accepts_dyn_filter = child_pushdown_result
- // .self_filters
- // .first()
- // .map(|filters| {
- // assert_eq_or_internal_err!(
- // filters.len(),
- // 1,
- // "Aggregate only pushdown one self dynamic filter"
- // );
- // let filter = filters.get(0).unwrap(); // Asserted above
- // Ok(matches!(filter.discriminant, PushedDown::Yes))
- // })
- // .unwrap_or_else(|| internal_err!("The length of self filters equals to the number of child of this ExecutionPlan, so it must be 1"))?;
-
- // HACK: The above snippet should be used, however, now the child reply
- // `PushDown::No` can indicate they're not able to push down row-level
- // filter, but still keep the filter for statistics pruning.
- // So here, we try to use ref count to determine if the dynamic filter
- // has actually be pushed down.
- // Issue: <https://github.com/apache/datafusion/issues/18856>
- let child_accepts_dyn_filter = Arc::strong_count(dyn_filter) > 1;
Review Comment:
Even if the rewrite to use `apply_expressions` looks like it adds a bunch of code, I think this is a nice use case for it, relying on the strong count of the dynamic filter's inner/outer `Arc` has caused several headaches before. Maybe we could do something similar in `HashJoinExec` to determine whether the dynamic filter has a consumer? Right now that's done via `is_used`, which relies on strong count. IIRC it's only used to check for consumers, so maybe we could remove `is_used` entirely in favor of this approach?
https://github.com/apache/datafusion/blob/3e3a92de29ed3d454e72c7bade6328508b6098c6/datafusion/physical-plan/src/joins/hash_join/exec.rs#L1385
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3729965517
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -2097,27 +2151,12 @@ impl ExecutionPlan for AggregateExec {
if phase == FilterPushdownPhase::Post
&& let Some(dyn_filter) = &self.dynamic_filter
{
- // let child_accepts_dyn_filter = child_pushdown_result
- // .self_filters
- // .first()
- // .map(|filters| {
- // assert_eq_or_internal_err!(
- // filters.len(),
- // 1,
- // "Aggregate only pushdown one self dynamic filter"
- // );
- // let filter = filters.get(0).unwrap(); // Asserted above
- // Ok(matches!(filter.discriminant, PushedDown::Yes))
- // })
- // .unwrap_or_else(|| internal_err!("The length of self filters equals to the number of child of this ExecutionPlan, so it must be 1"))?;
-
- // HACK: The above snippet should be used, however, now the child reply
- // `PushDown::No` can indicate they're not able to push down row-level
- // filter, but still keep the filter for statistics pruning.
- // So here, we try to use ref count to determine if the dynamic filter
- // has actually be pushed down.
- // Issue: <https://github.com/apache/datafusion/issues/18856>
- let child_accepts_dyn_filter = Arc::strong_count(dyn_filter) > 1;
Review Comment:
Done.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5241341186
> Should GlobalLimitExec::apply_expressions traverse GlobalLimitExec::required_ordering?
@adriangb I thought about these cases and decided that expressions in `PlanProperties` should be out of scope for `apply_expressions`. More justification [here](https://github.com/apache/datafusion/pull/24018#discussion_r3742124344)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3729421662
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -165,6 +165,55 @@ where string types are preferred (`UNION`, `CASE THEN/ELSE`, `NVL2`).
string-preferring behavior
- Crates that call `get_coerce_type_for_case_expression`
+### `ExecutionPlan::apply_expressions` is now a required method
Review Comment:
Right, moved.
##########
datafusion/physical-plan/src/async_func.rs:
##########
Review Comment:
I missed this. Just added it
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -165,6 +165,55 @@ where string types are preferred (`UNION`, `CASE THEN/ELSE`, `NVL2`).
string-preferring behavior
- Crates that call `get_coerce_type_for_case_expression`
+### `ExecutionPlan::apply_expressions` is now a required method
+
+`apply_expressions` has been added as a **required** method on the `ExecutionPlan` trait (no default implementation). The same applies to the `FileSource` and `DataSource` traits. Any custom implementation of these traits must now implement `apply_expressions`.
+
+**Who is affected:**
+
+- Users who implement custom `ExecutionPlan` nodes
+- Users who implement custom `FileSource` or `DataSource` sources
+
+**Migration guide:**
+
+Add `apply_expressions` to your implementation. Call `f` on each top-level
+`PhysicalExpr` your node owns. Do not recurse into child plans or expression
+children. Use `apply_expression_roots` for nodes with expressions so `Stop`
+short-circuits the iteration and `Jump` proceeds to the next root expression.
+
+**Node with no expressions:**
+
+```rust,ignore
+fn apply_expressions(
+ &self,
+ _f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+) -> Result<TreeNodeRecursion> {
+ Ok(TreeNodeRecursion::Continue)
Review Comment:
`apply_expression_roots(std::iter::empty(), f)` is nice, but you have to use type annotations in practice:
```
apply_expression_roots(
std::iter::empty::<&Arc<dyn PhysicalExpr>>(),
f,
)
```
This is a bit less clean so
I added a helper `apply_no_expressions` instead.
##########
datafusion/physical-plan/src/streaming.rs:
##########
Review Comment:
I missed this. Just added it.
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -1337,6 +1338,20 @@ impl ExecutionPlan for RepartitionExec {
vec![&self.input]
}
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ match self.partitioning() {
+ Partitioning::Hash(exprs, _) => crate::apply_expression_roots(exprs, f),
Review Comment:
I think I just missed the expressions in `StreamingTableExec`. Will update it.
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -763,6 +764,32 @@ struct AggrDynFilter {
supported_accumulators_info: Vec<PerAccumulatorDynFilter>,
}
+fn plan_contains_expression_id(
+ plan: &Arc<dyn ExecutionPlan>,
+ expression_id: u64,
+) -> Result<bool> {
+ let mut found = false;
+ plan.apply(|node| {
+ node.apply_expressions(&mut |root| {
+ root.apply(|expr| {
Review Comment:
I think this pattern is expected. I followed these docs for `apply_expressions` on logical plans to implement this for physical plans: https://github.com/apache/datafusion/blob/5514194fa7362e7fbc2e3f891779e5df90f57322/datafusion/expr/src/logical_plan/plan.rs?plain=1#L135-L144.
##########
datafusion/datasource/src/file.rs:
##########
@@ -351,6 +352,27 @@ pub trait FileSource: Any + Send + Sync {
fn schema_adapter_factory(&self) -> Option<Arc<dyn SchemaAdapterFactory>> {
None
}
+
+ /// Apply a function to all physical expressions used by this file source.
+ ///
+ /// This includes:
+ /// - Filter predicates (which may contain dynamic filters)
+ /// - Projection expressions
+ ///
+ /// The function `f` is called once for each expression. The function should
+ /// return `TreeNodeRecursion::Continue` to continue visiting other expressions,
+ /// or `TreeNodeRecursion::Stop` to stop visiting expressions early.
Review Comment:
Fixed!
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5221848830
Sorry hit submit too early -- I am finishing my review now
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3748717296
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
Also, the Exprs in properties are not actually **evaluated** because they are part of the properites, but rather are book keeping the result of that evaluation.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5221952188
@alamb Thanks. I'll let you finish the review. Also I can fix up the commits! I figured I'd wait for a review first
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5239027883
I am merging this one in so we can have downstream projects test with it briefly
Thanks again @jayshrivastava and @gene-bordegaray and @LiaCastaneda
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5137650017
I see the code coverage is isn't very high. I don't see any good candidates to call `apply_expressions` generally. Maybe we can implement `check_invariants` on a few `ExecutionPlan` implementations which assert `apply_expressions` returns the right number of expressions.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5135862782
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v54.1.0 (current)
Built [ 124.970s] (current)
Parsing datafusion v54.1.0 (current)
Parsed [ 0.038s] (current)
Building datafusion v54.1.0 (baseline)
Built [ 118.362s] (baseline)
Parsing datafusion v54.1.0 (baseline)
Parsed [ 0.041s] (baseline)
Checking datafusion v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.966s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 246.454s] datafusion
Building datafusion-catalog v54.1.0 (current)
Built [ 43.286s] (current)
Parsing datafusion-catalog v54.1.0 (current)
Parsed [ 0.027s] (current)
Building datafusion-catalog v54.1.0 (baseline)
Built [ 42.636s] (baseline)
Parsing datafusion-catalog v54.1.0 (baseline)
Parsed [ 0.028s] (baseline)
Checking datafusion-catalog v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.158s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 87.199s] datafusion-catalog
Building datafusion-datasource v54.1.0 (current)
Built [ 43.625s] (current)
Parsing datafusion-datasource v54.1.0 (current)
Parsed [ 0.036s] (current)
Building datafusion-datasource v54.1.0 (baseline)
Built [ 43.476s] (baseline)
Parsing datafusion-datasource v54.1.0 (baseline)
Parsed [ 0.037s] (baseline)
Checking datafusion-datasource v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.365s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure trait_method_added: pub trait method added ---
Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/trait_method_added.ron
Failed in:
trait method datafusion_datasource::source::DataSource::apply_expressions in file /home/runner/work/datafusion/datafusion/datafusion/datasource/src/source.rs:240
trait method datafusion_datasource::file::FileSource::apply_expressions in file /home/runner/work/datafusion/datafusion/datafusion/datasource/src/file.rs:372
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 88.717s] datafusion-datasource
Building datafusion-datasource-arrow v54.1.0 (current)
Built [ 42.337s] (current)
Parsing datafusion-datasource-arrow v54.1.0 (current)
Parsed [ 0.012s] (current)
Building datafusion-datasource-arrow v54.1.0 (baseline)
Built [ 42.621s] (baseline)
Parsing datafusion-datasource-arrow v54.1.0 (baseline)
Parsed [ 0.013s] (baseline)
Checking datafusion-datasource-arrow v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.098s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 86.120s] datafusion-datasource-arrow
Building datafusion-datasource-avro v54.1.0 (current)
Built [ 43.789s] (current)
Parsing datafusion-datasource-avro v54.1.0 (current)
Parsed [ 0.011s] (current)
Building datafusion-datasource-avro v54.1.0 (baseline)
Built [ 43.590s] (baseline)
Parsing datafusion-datasource-avro v54.1.0 (baseline)
Parsed [ 0.011s] (baseline)
Checking datafusion-datasource-avro v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.102s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 88.718s] datafusion-datasource-avro
Building datafusion-datasource-csv v54.1.0 (current)
Built [ 42.638s] (current)
Parsing datafusion-datasource-csv v54.1.0 (current)
Parsed [ 0.012s] (current)
Building datafusion-datasource-csv v54.1.0 (baseline)
Built [ 42.766s] (baseline)
Parsing datafusion-datasource-csv v54.1.0 (baseline)
Parsed [ 0.012s] (baseline)
Checking datafusion-datasource-csv v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.143s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 86.635s] datafusion-datasource-csv
Building datafusion-datasource-json v54.1.0 (current)
Built [ 43.173s] (current)
Parsing datafusion-datasource-json v54.1.0 (current)
Parsed [ 0.013s] (current)
Building datafusion-datasource-json v54.1.0 (baseline)
Built [ 42.984s] (baseline)
Parsing datafusion-datasource-json v54.1.0 (baseline)
Parsed [ 0.014s] (baseline)
Checking datafusion-datasource-json v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.120s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 87.389s] datafusion-datasource-json
Building datafusion-datasource-parquet v54.1.0 (current)
Built [ 48.670s] (current)
Parsing datafusion-datasource-parquet v54.1.0 (current)
Parsed [ 0.034s] (current)
Building datafusion-datasource-parquet v54.1.0 (baseline)
Built [ 49.006s] (baseline)
Parsing datafusion-datasource-parquet v54.1.0 (baseline)
Parsed [ 0.037s] (baseline)
Checking datafusion-datasource-parquet v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.218s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 99.225s] datafusion-datasource-parquet
Building datafusion-ffi v54.1.0 (current)
Built [ 65.514s] (current)
Parsing datafusion-ffi v54.1.0 (current)
Parsed [ 0.068s] (current)
Building datafusion-ffi v54.1.0 (baseline)
Built [ 65.066s] (baseline)
Parsing datafusion-ffi v54.1.0 (baseline)
Parsed [ 0.067s] (baseline)
Checking datafusion-ffi v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.360s] 223 checks: 221 pass, 1 fail, 1 warn, 30 skip
--- failure constructible_struct_adds_field: externally-constructible struct adds field ---
Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/constructible_struct_adds_field.ron
Failed in:
field FFI_PhysicalExpr.expression_id in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:123
field ForeignLibraryModule.create_exec_with_expressions in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:112
field FFI_ExecutionPlan.apply_expressions in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:56
field FFI_ExecutionPlan.version in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:101
--- warning repr_c_plain_struct_fields_reordered: struct fields reordered in repr(C) struct ---
Description:
A public repr(C) struct had its fields reordered. This can change the struct's memory layout, possibly breaking FFI use cases that depend on field position and order.
ref: https://doc.rust-lang.org/reference/type-layout.html#reprc-structs
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/repr_c_plain_struct_fields_reordered.ron
Failed in:
FFI_ExecutionPlan.with_new_children moved from position 3 to 4, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:59
FFI_ExecutionPlan.name moved from position 4 to 5, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:63
FFI_ExecutionPlan.execute moved from position 5 to 6, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:67
FFI_ExecutionPlan.repartitioned moved from position 6 to 7, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:73
FFI_ExecutionPlan.metrics moved from position 7 to 8, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:82
FFI_ExecutionPlan.partition_statistics moved from position 8 to 9, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:88
FFI_ExecutionPlan.clone moved from position 9 to 10, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:95
FFI_ExecutionPlan.release moved from position 10 to 11, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:98
FFI_ExecutionPlan.private_data moved from position 11 to 13, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:105
FFI_ExecutionPlan.library_marker_id moved from position 12 to 14, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/execution_plan.rs:110
FFI_PhysicalExpr.display moved from position 17 to 18, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:126
FFI_PhysicalExpr.hash moved from position 18 to 19, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:129
FFI_PhysicalExpr.clone moved from position 19 to 20, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:133
FFI_PhysicalExpr.release moved from position 20 to 21, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:136
FFI_PhysicalExpr.version moved from position 21 to 22, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:139
FFI_PhysicalExpr.private_data moved from position 22 to 23, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:143
FFI_PhysicalExpr.library_marker_id moved from position 23 to 24, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:147
ForeignLibraryModule.create_exec_with_statistics moved from position 15 to 16, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:114
ForeignLibraryModule.create_table_with_statistics moved from position 16 to 17, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:116
ForeignLibraryModule.create_physical_optimizer_rule moved from position 17 to 18, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:119
ForeignLibraryModule.create_context_aware_optimizer_rule moved from position 18 to 19, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:121
ForeignLibraryModule.version moved from position 19 to 20, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/tests/mod.rs:123
Summary semver requires new major version: 1 major and 0 minor checks failed
Warning produced 1 major and 0 minor level warnings
Finished [ 132.502s] datafusion-ffi
Building datafusion-physical-optimizer v54.1.0 (current)
Built [ 43.389s] (current)
Parsing datafusion-physical-optimizer v54.1.0 (current)
Parsed [ 0.023s] (current)
Building datafusion-physical-optimizer v54.1.0 (baseline)
Built [ 43.782s] (baseline)
Parsing datafusion-physical-optimizer v54.1.0 (baseline)
Parsed [ 0.024s] (baseline)
Checking datafusion-physical-optimizer v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.163s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 88.461s] datafusion-physical-optimizer
Building datafusion-physical-plan v54.1.0 (current)
Built [ 40.417s] (current)
Parsing datafusion-physical-plan v54.1.0 (current)
Parsed [ 0.156s] (current)
Building datafusion-physical-plan v54.1.0 (baseline)
Built [ 40.925s] (baseline)
Parsing datafusion-physical-plan v54.1.0 (baseline)
Parsed [ 0.155s] (baseline)
Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.963s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure trait_method_added: pub trait method added ---
Description:
A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait
ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/trait_method_added.ron
Failed in:
trait method datafusion_physical_plan::execution_plan::ExecutionPlan::apply_expressions in file /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/execution_plan.rs:322
trait method datafusion_physical_plan::ExecutionPlan::apply_expressions in file /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/execution_plan.rs:322
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 83.848s] datafusion-physical-plan
Building datafusion-proto v54.1.0 (current)
Built [ 65.460s] (current)
Parsing datafusion-proto v54.1.0 (current)
Parsed [ 0.019s] (current)
Building datafusion-proto v54.1.0 (baseline)
Built [ 66.541s] (baseline)
Parsing datafusion-proto v54.1.0 (baseline)
Parsed [ 0.021s] (baseline)
Checking datafusion-proto v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.374s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 135.037s] datafusion-proto
```
</details>
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3721412066
##########
datafusion/datasource/src/file.rs:
##########
@@ -351,6 +352,27 @@ pub trait FileSource: Any + Send + Sync {
fn schema_adapter_factory(&self) -> Option<Arc<dyn SchemaAdapterFactory>> {
None
}
+
+ /// Apply a function to all physical expressions used by this file source.
+ ///
+ /// This includes:
+ /// - Filter predicates (which may contain dynamic filters)
+ /// - Projection expressions
+ ///
+ /// The function `f` is called once for each expression. The function should
+ /// return `TreeNodeRecursion::Continue` to continue visiting other expressions,
+ /// or `TreeNodeRecursion::Stop` to stop visiting expressions early.
Review Comment:
this is outdated no? we can use `apply_expression_roots` now
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
yeah I think its fine to leave it as is, I think a helper for this would be too much
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -165,6 +165,55 @@ where string types are preferred (`UNION`, `CASE THEN/ELSE`, `NVL2`).
string-preferring behavior
- Crates that call `get_coerce_type_for_case_expression`
+### `ExecutionPlan::apply_expressions` is now a required method
+
+`apply_expressions` has been added as a **required** method on the `ExecutionPlan` trait (no default implementation). The same applies to the `FileSource` and `DataSource` traits. Any custom implementation of these traits must now implement `apply_expressions`.
+
+**Who is affected:**
+
+- Users who implement custom `ExecutionPlan` nodes
+- Users who implement custom `FileSource` or `DataSource` sources
+
+**Migration guide:**
+
+Add `apply_expressions` to your implementation. Call `f` on each top-level
+`PhysicalExpr` your node owns. Do not recurse into child plans or expression
+children. Use `apply_expression_roots` for nodes with expressions so `Stop`
+short-circuits the iteration and `Jump` proceeds to the next root expression.
+
+**Node with no expressions:**
+
+```rust,ignore
+fn apply_expressions(
+ &self,
+ _f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+) -> Result<TreeNodeRecursion> {
+ Ok(TreeNodeRecursion::Continue)
Review Comment:
Since we already have `apply_expression_roots` to abstract "walk these expressions" case, would it be worth a small sibling helper for the "I have none" case? Right now that's just `Ok(TreeNodeRecursion::Continue)` repeated at every leaf/no-op node. Wondering if it would make sense to have something like `apply_expression_continue()` living next to it, just a plain function.
I think something like `apply_expression_roots(std::iter::empty(), f)` does the same thing, but since we're trying to abstract away `TreeNodeRecursion`, wondering if it would still make sense to have the dedicated helper
##########
datafusion/physical-plan/src/async_func.rs:
##########
Review Comment:
+1 on this, looks like something I missed on the initial `apply_expressions` PR
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3736189205
##########
datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:
##########
@@ -423,22 +423,6 @@ impl DynamicFilterPhysicalExpr {
}
}
- /// Check if this dynamic filter is being actively used by any consumers.
- ///
- /// Returns `true` if there are references beyond the producer (e.g., the HashJoinExec
- /// that created the filter). This is useful to avoid computing expensive filter
- /// expressions when no consumer will actually use them.
- ///
- /// # Implementation Details
- ///
- /// We check both Arc counts to handle two cases:
- /// - Transformed filters (via `with_new_children`) share the inner Arc (inner count > 1)
- /// - Direct clones (via `Arc::clone`) increment the outer count (outer count > 1)
- pub fn is_used(self: &Arc<Self>) -> bool {
Review Comment:
Done
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5222231624
> @alamb Thanks. I'll let you finish the review. Also I can fix up the commits! I figured I'd wait for a review first
Thanks @jayshrivastava -- I am done, though it appears I (claude) botched the merge. Will fix
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3741954555
##########
datafusion/physical-plan/src/execution_plan.rs:
##########
@@ -247,6 +250,62 @@ pub trait ExecutionPlan: Any + Debug + DisplayAs + Send + Sync {
/// joins).
fn children(&self) -> Vec<&Arc<dyn ExecutionPlan>>;
+ /// Apply a closure `f` to each expression in the current physical plan node. `f`
+ /// should not be called in any child expressions nor in any expressions of child nodes.
+ ///
+ /// The closure can return [`TreeNodeRecursion::Stop`] to stop iteration, otherwise
+ /// iteration should continue. ([`TreeNodeRecursion::Jump`] and [`TreeNodeRecursion::Jump`]
Review Comment:
Looks like this was resolved
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5218219386
@cetra3 Imo, apply_expressions isn't leaking internal state, at least not to the extent prior to https://github.com/apache/datafusion/issues/23494. It's just providing iteration over expressions in a plan. It also mirrors the API for logical plan nodes https://github.com/apache/datafusion/blob/db63637a92c683a89adee67ee04ae5c42992fd63/datafusion/expr/src/logical_plan/tree_node.rs?plain=1#L418-L421
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3736738128
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
I believe should we apply expressions to this op because of this as well?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5241957629
@alamb Not urgent, but I tried implementing the refactor [here](https://github.com/apache/datafusion/pull/24226/changes#r3750579635).
`AsRef<Arc<dyn PhysicalExpr>>` isn't implemented for `Arc<dyn PhysicalExpr>` surprisingly, so we cannot cleanly do the refactor you mentioned. Let me know what you think.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3743596783
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
yes this cleras things up, was moslty getting confused from documentationon when exactly we plan to use this and not but this clears things up π
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3729717085
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -2097,27 +2151,12 @@ impl ExecutionPlan for AggregateExec {
if phase == FilterPushdownPhase::Post
&& let Some(dyn_filter) = &self.dynamic_filter
{
- // let child_accepts_dyn_filter = child_pushdown_result
- // .self_filters
- // .first()
- // .map(|filters| {
- // assert_eq_or_internal_err!(
- // filters.len(),
- // 1,
- // "Aggregate only pushdown one self dynamic filter"
- // );
- // let filter = filters.get(0).unwrap(); // Asserted above
- // Ok(matches!(filter.discriminant, PushedDown::Yes))
- // })
- // .unwrap_or_else(|| internal_err!("The length of self filters equals to the number of child of this ExecutionPlan, so it must be 1"))?;
-
- // HACK: The above snippet should be used, however, now the child reply
- // `PushDown::No` can indicate they're not able to push down row-level
- // filter, but still keep the filter for statistics pruning.
- // So here, we try to use ref count to determine if the dynamic filter
- // has actually be pushed down.
- // Issue: <https://github.com/apache/datafusion/issues/18856>
- let child_accepts_dyn_filter = Arc::strong_count(dyn_filter) > 1;
Review Comment:
This is a good idea! I'll do it rn
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "cetra3 (via GitHub)" <gi...@apache.org>.
cetra3 commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5211090555
This PR as it stands is in conflict with https://github.com/apache/datafusion/issues/23494. I think it may need a pass over it to align with the `try_to_proto` way of doing things.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5241285741
Thanks for merging!
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3740377948
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
yes but is using `apply_no_expressions(f)`
I was thinking that since this method can add the ordering to the cached `self.cache.eq_properties` we would traverse that in the `apply_expressions()` method here.
Maybe I am still unclear about what needs to be traversed and not @jayshrivastava ?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3740380634
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
would this also be in the same boat? we would need to traverse these?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3748708956
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
> Good point. The behavior in this PR is inconsistent, so I just pushed a commit with a comment to define the behavior: I think we should ignore expressions in the properties() for several reasons
I agree -- I think it is unlikely that expressions will appear in properties that do not also appear in other expressions of the node (e.g. the exprs in a ProjectionExec)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3687020759
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
Question for reviewers - this pattern repeats a lot. Is it worth adding a helper?
```rust
apply_expression_roots(
properties
.output_ordering()
.into_iter()
.flatten()
.map(|sort_expr| &sort_expr.expr),
f
)
```
The projection pattern also repeats a bit
```rust
crate::apply_expression_roots(
self.projector
.projection()
.as_ref()
.iter()
.map(|proj_expr| &proj_expr.expr),
f,
)
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3719496719
##########
datafusion/physical-plan/src/async_func.rs:
##########
Review Comment:
why do we just continue here and not apply on these?
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -165,6 +165,55 @@ where string types are preferred (`UNION`, `CASE THEN/ELSE`, `NVL2`).
string-preferring behavior
- Crates that call `get_coerce_type_for_case_expression`
+### `ExecutionPlan::apply_expressions` is now a required method
Review Comment:
I think this should b in the 55.0.0 release
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -1337,6 +1338,20 @@ impl ExecutionPlan for RepartitionExec {
vec![&self.input]
}
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ match self.partitioning() {
+ Partitioning::Hash(exprs, _) => crate::apply_expression_roots(exprs, f),
Review Comment:
We do the apply_expression_roots here for the stored partitioning. But then in something like the `StreamingTableExec` we drop certain expressions, what are the exact rules around this?
##########
datafusion-examples/examples/relation_planner/table_sample.rs:
##########
@@ -749,6 +749,22 @@ impl ExecutionPlan for SampleExec {
Ok(Arc::new(stats))
}
+
+ fn apply_expressions(
+ &self,
+ f: &mut dyn FnMut(
+ &Arc<dyn datafusion::physical_plan::PhysicalExpr>,
+ ) -> Result<TreeNodeRecursion>,
+ ) -> Result<TreeNodeRecursion> {
+ datafusion::physical_plan::apply_expression_roots(
+ self.cache
+ .output_ordering()
+ .into_iter()
+ .flatten()
+ .map(|sort_expr| &sort_expr.expr),
+ f,
+ )
Review Comment:
I think that would be too much indirection IMO, others might have some opinions though
##########
datafusion/physical-plan/src/streaming.rs:
##########
Review Comment:
should we be applying expressions here too since they are owned by the operator?
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -763,6 +764,32 @@ struct AggrDynFilter {
supported_accumulators_info: Vec<PerAccumulatorDynFilter>,
}
+fn plan_contains_expression_id(
+ plan: &Arc<dyn ExecutionPlan>,
+ expression_id: u64,
+) -> Result<bool> {
+ let mut found = false;
+ plan.apply(|node| {
+ node.apply_expressions(&mut |root| {
+ root.apply(|expr| {
Review Comment:
is there any way to eliminate this triple nesting we do.
From my understanding its:
1. walking the execution plan tree
2. Walk the expressions roots in one plan node
3. Walks inside the one expression
Maybe something like a `apply_plan_expressions` would make this easier to read?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5135902701
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24018?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `39.68433%` with `535 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 80.76%. Comparing base ([`541caab`](https://app.codecov.io/gh/apache/datafusion/commit/541caab85a265c104888f1988fe54b08fa022f53?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8eff543`](https://app.codecov.io/gh/apache/datafusion/commit/8eff543fc574f87398bd257cb49ec05127d6b17f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 4 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24018?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-plan/src/execution\_plan.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fexecution_plan.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9leGVjdXRpb25fcGxhbi5ycw==) | 51.03% | [66 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/test/exec.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Ftest%2Fexec.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy90ZXN0L2V4ZWMucnM=) | 0.00% | [36 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/core/src/physical\_planner.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fcore%2Fsrc%2Fphysical_planner.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9waHlzaWNhbF9wbGFubmVyLnJz) | 0.00% | [24 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/aggregates/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL21vZC5ycw==) | 71.08% | [12 Missing and 12 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...sion/physical-optimizer/src/output\_requirements.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-optimizer%2Fsrc%2Foutput_requirements.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1vcHRpbWl6ZXIvc3JjL291dHB1dF9yZXF1aXJlbWVudHMucnM=) | 0.00% | [22 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/limit.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Flimit.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9saW1pdC5ycw==) | 0.00% | [22 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/sorts/sort.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fsorts%2Fsort.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9zb3J0cy9zb3J0LnJz) | 0.00% | [20 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/display.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fdisplay.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9kaXNwbGF5LnJz) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...n/physical-plan/src/sorts/sort\_preserving\_merge.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fsorts%2Fsort_preserving_merge.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9zb3J0cy9zb3J0X3ByZXNlcnZpbmdfbWVyZ2UucnM=) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ysical-plan/src/windows/bounded\_window\_agg\_exec.rs](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fwindows%2Fbounded_window_agg_exec.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy93aW5kb3dzL2JvdW5kZWRfd2luZG93X2FnZ19leGVjLnJz) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [36 more](https://app.codecov.io/gh/apache/datafusion/pull/24018?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24018 +/- ##
==========================================
- Coverage 80.84% 80.76% -0.09%
==========================================
Files 1096 1099 +3
Lines 373936 375224 +1288
Branches 373936 375224 +1288
==========================================
+ Hits 302313 303039 +726
- Misses 53584 54101 +517
- Partials 18039 18084 +45
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24018?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3686973334
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -3085,6 +3085,111 @@ async fn test_filter_with_projection_pushdown() {
assert_batches_eq!(expected, &result);
}
+/// Test that ExecutionPlan::apply_expressions() can discover dynamic filters across the plan tree.
+///
+/// Not portable to sqllogictest: asserts by walking the plan tree with
+/// `apply_expressions` + `downcast_ref::<DynamicFilterPhysicalExpr>` and
+/// counting nodes. Neither API is observable from SQL.
+#[tokio::test]
+async fn test_discover_dynamic_filters_via_expressions_api() {
Review Comment:
This test explicitly covers that dynamic filters are discoverable post-pushdown. This is what we want in datafusion-distributed and I assume other projects want this as well
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -2506,6 +2545,8 @@ impl AggregateExec {
})?;
aggregate.with_dynamic_filter_expr(dynamic_filter)?
} else {
+ let mut aggregate = aggregate;
+ aggregate.dynamic_filter = None;
Review Comment:
Semi-related. I think this was a bug. We kept the default dynamic filter created by `try_new` around even there's no pushed down filter. Now, we explicitly remove it. The test I added in `datafusion/proto/tests/cases/roundtrip_physical_plan.rs` covers this.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3736477513
##########
docs/source/library-user-guide/upgrading/54.0.0.md:
##########
@@ -165,6 +165,55 @@ where string types are preferred (`UNION`, `CASE THEN/ELSE`, `NVL2`).
string-preferring behavior
- Crates that call `get_coerce_type_for_case_expression`
+### `ExecutionPlan::apply_expressions` is now a required method
+
+`apply_expressions` has been added as a **required** method on the `ExecutionPlan` trait (no default implementation). The same applies to the `FileSource` and `DataSource` traits. Any custom implementation of these traits must now implement `apply_expressions`.
+
+**Who is affected:**
+
+- Users who implement custom `ExecutionPlan` nodes
+- Users who implement custom `FileSource` or `DataSource` sources
+
+**Migration guide:**
+
+Add `apply_expressions` to your implementation. Call `f` on each top-level
+`PhysicalExpr` your node owns. Do not recurse into child plans or expression
+children. Use `apply_expression_roots` for nodes with expressions so `Stop`
+short-circuits the iteration and `Jump` proceeds to the next root expression.
+
+**Node with no expressions:**
+
+```rust,ignore
+fn apply_expressions(
+ &self,
+ _f: &mut dyn FnMut(&Arc<dyn PhysicalExpr>) -> Result<TreeNodeRecursion>,
+) -> Result<TreeNodeRecursion> {
+ Ok(TreeNodeRecursion::Continue)
Review Comment:
Jay (or anyone), feel free to disagree. I suggested this because imo itβs clearer to me, but whatever is clearest when upgrading is best.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5229043243
Addressed comments. I have to run but here are the remaining open threads of work/discussion
1. Fix conflicts - I will do later
2. Small discussion about if expressions in the `properties()` should be traversed by `apply_expressions`. I think they should not. https://github.com/apache/datafusion/pull/24018#discussion_r3742124344
3. The [AsRef](https://github.com/apache/datafusion/pull/24018#pullrequestreview-4886401724) follow up. I actually wouldn't mind doing this in this PR.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "jayshrivastava (via GitHub)" <gi...@apache.org>.
jayshrivastava commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3742124344
##########
datafusion/physical-plan/src/memory.rs:
##########
Review Comment:
Good point. The behavior in this PR is inconsistent, so I just pushed a commit with a comment to define the behavior: I think we should ignore expressions in the `properties()` for several reasons
1. It's a lot of code to have every `ExecutionPlan` has to visit the sort expr and the partitioning expr in the properties
2. Many `ExecutionPlan` nodes just return `self.children().properties()`, so it's redundant
3. I doubt users will want `apply_expressions` to yield expressions in properties. I think users would only care about expressions that are evaluated and important during execution.
4. You can already call `properties()` if you want the properties
Here's the updated doc comment:
```
/// Apply a closure `f` to each root expression that this node owns and uses
/// during execution, either by evaluating it or updating it dynamically.
///
/// An expression must not be visited solely because it describes an input or
/// output property, such as cached ordering, partitioning, or equivalence
/// metadata. However, these may be traversed indirectly. For example,
/// `RepartitionExec` visits the partitioning expressions it evaluates and
/// `SortExec` visits the sort expressions it evaluates to order rows.
///
/// This method is shallow: it must not visit expression children or expressions
/// owned by child execution plans.
///
/// Similarly to other [`TreeNode`] APIs, the closure can return
```
Let me know if this makes sense.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "LiaCastaneda (via GitHub)" <gi...@apache.org>.
LiaCastaneda commented on code in PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#discussion_r3734330148
##########
datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:
##########
@@ -423,22 +423,6 @@ impl DynamicFilterPhysicalExpr {
}
}
- /// Check if this dynamic filter is being actively used by any consumers.
- ///
- /// Returns `true` if there are references beyond the producer (e.g., the HashJoinExec
- /// that created the filter). This is useful to avoid computing expensive filter
- /// expressions when no consumer will actually use them.
- ///
- /// # Implementation Details
- ///
- /// We check both Arc counts to handle two cases:
- /// - Transformed filters (via `with_new_children`) share the inner Arc (inner count > 1)
- /// - Direct clones (via `Arc::clone`) increment the outer count (outer count > 1)
- pub fn is_used(self: &Arc<Self>) -> bool {
Review Comment:
should we mark it as deprecated instead?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5239444186
Should `GlobalLimitExec::apply_expressions` traverse `GlobalLimitExec::required_ordering`?
https://github.com/apache/datafusion/blob/2bfdd4aea181525f5328781b29174d82be0c7fd4/datafusion/physical-plan/src/limit.rs#L60
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "gene-bordegaray (via GitHub)" <gi...@apache.org>.
gene-bordegaray commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5241064044
> I am merging this one in so we can have downstream projects test with it briefly
>
> Thanks again @jayshrivastava and @gene-bordegaray and @LiaCastaneda
@alamb ok sounds good, thank you and we will be testing this as well π
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Reapply "Add ExecutionPlan::apply_expressions() (apache#20337)" (apache#22437) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24018:
URL: https://github.com/apache/datafusion/pull/24018#issuecomment-5239034644
> The https://github.com/apache/datafusion/pull/24018#pullrequestreview-4886401724 follow up.
A small quick follow on PR would be good too
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org