adriangb commented on code in PR #15568:
URL: https://github.com/apache/datafusion/pull/15568#discussion_r2036221582
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -283,6 +284,51 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug +
DynEq + DynHash {
/// See the [`fmt_sql`] function for an example of printing
`PhysicalExpr`s as SQL.
///
fn fmt_sql(&self, f: &mut Formatter<'_>) -> fmt::Result;
+
+ /// Take a snapshot of this `PhysicalExpr` if it is dynamic.
+ /// This is used to capture the current state of `PhysicalExpr`s that may
contain
+ /// dynamic references to other operators in order to serialize it over
the wire
+ /// or treat it via downcast matching.
+ ///
+ /// You should not call this method directly as it does not handle
recursion.
+ /// Instead use [`snapshot_physical_expr`] to handle recursion and capture
the
+ /// full state of the `PhysicalExpr`.
+ ///
+ /// This is expected to return "simple" expressions that do not have
mutable state
+ /// and are composed of DataFusion's built-in `PhysicalExpr`
implementations.
+ /// Callers however should *not* assume anything about the returned
expressions
+ /// since callers and implementers may not agree on what "simple" or
"built-in"
+ /// means.
+ /// In other words, if you need to searlize a `PhysicalExpr` across the
wire
+ /// you should call this method and then try to serialize the result,
+ /// but you should handle unknown or unexpected `PhysicalExpr`
implementations gracefully
+ /// just as if you had not called this method at all.
+ ///
+ /// In particular, consider:
+ /// * A `PhysicalExpr` that references the current state of a
`datafusion::physical_plan::TopK`
+ /// that is involved in a query with `SELECT * FROM t1 ORDER BY a LIMIT
10`.
+ /// This function may return something like `a >= 12`.
+ /// * A `PhysicalExpr` that references the current state of a
`datafusion::physical_plan::joins::HashJoinExec`
+ /// from a query such as `SELECT * FROM t1 JOIN t2 ON t1.a = t2.b`.
+ /// This function may return something like `t2.b IN (1, 5, 7)`.
+ ///
+ /// A system or function that can only deal with a hardcoded set of
`PhysicalExpr` implementations
+ /// or needs to serialize this state to bytes may not be able to handle
these dynamic references.
+ /// In such cases, we should return a simplified version of the
`PhysicalExpr` that does not
+ /// contain these dynamic references.
+ ///
+ /// Systems that implement remote execution of plans, e.g. serialize a
portion of the query plan
+ /// and send it across the wire to a remote executor may want to call this
method after
+ /// every batch on the source side and brodcast / update the current
snaphot to the remote executor.
+ ///
+ /// Note for implementers: this method should *not* handle recursion.
+ /// Recursion is handled in [`snapshot_physical_expr`].
+ fn snapshot(&self) -> Result<Option<Arc<dyn PhysicalExpr>>> {
+ // By default, we return None to indicate that this PhysicalExpr does
not
+ // have any dynamic references or state.
+ // This is a safe default behavior.
+ Ok(None)
+ }
Review Comment:
Is that true? Where would it hold onto the reference from, `data_type()`?
Can't the same `PhysicalExpr` be used in multiple contexts, e.g. with the same
columns but different projections / different physical column orders? Put
another way, is it guaranteed that `data_type()` can only be called once or
that `evaluate()` is always called with the same schema?
To be clear: I don't think this is important for this PR. After reflection
in https://github.com/apache/datafusion/pull/15568/files#r2033710338 I think if
we want that we can add it as a new method in
https://github.com/apache/datafusion/pull/15057
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
