adriangb commented on code in PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#discussion_r3255038319
##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -477,6 +477,95 @@ pub trait PhysicalExpr: Any + Send + Sync + Display +
Debug + DynEq + DynHash {
fn expression_id(&self) -> Option<u64> {
None
}
+
+ /// Serialize this expression to a [`PhysicalExprNode`] proto message.
+ ///
+ /// Returning `Ok(None)` means "this expression does not know how to
+ /// serialize itself"; the caller (typically `datafusion-proto`) will fall
+ /// back to its existing codec / extension paths. This matches today's
+ /// behavior for expressions that aren't built into `datafusion-proto`.
+ ///
+ /// Returning `Ok(Some(node))` means the expression has serialized itself
+ /// fully; the caller should not try any further fallback path.
+ ///
+ /// Returning `Err(_)` means a real serialization failure (e.g. the
+ /// expression knows it should serialize but a child failed).
+ ///
+ /// The motivating use case is letting expressions with private state
+ /// (e.g. `DynamicFilterPhysicalExpr`'s `RwLock`-protected inner fields)
+ /// reach into their own internals for `try_to_proto`/`try_from_proto`
+ /// without having to expose `pub` accessors to `datafusion-proto`. See
+ /// <https://github.com/apache/datafusion/issues/21835>.
+ ///
+ /// The `try_` prefix matches the fallible `try_from_proto` decode
+ /// constructors (and the `TryFromProto` trait in `datafusion-proto`);
+ /// both sides of the round-trip are fallible and named consistently.
+ ///
+ /// [`PhysicalExprNode`]:
datafusion_proto_models::protobuf::PhysicalExprNode
+ #[cfg(feature = "proto")]
+ fn try_to_proto(
+ &self,
+ _ctx: &proto_encode::PhysicalExprEncodeCtx<'_>,
+ ) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {
Review Comment:
@timsaucer do you think we should make this `Result<...>` instead of
`Result<Option<...>>`? If we ever plan to remove the default impl, remove the
codecs and force ourselves to implement this method on all expressions we
produce then the default returning an error would make sense IMO. The price we
pay is that in the meantime it would be hard to differentiate between "I called
an expression with an implementation and it error, I should bubble that up to
users so they can fix it instead of getting an `unknown expression Column` type
error (by falling back to the match -> codec, etc.)" vs. "this expression
hasn't been updated yet".
--
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]
