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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
