kumarUjjawal commented on code in PR #23495:
URL: https://github.com/apache/datafusion/pull/23495#discussion_r3585720444
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -731,36 +747,6 @@ pub trait PhysicalPlanNodeExt: Sized {
)))
}
- fn try_into_projection_physical_plan(
Review Comment:
wouldn't removing these both `try_into_projection_physical_plan` and
`try_from_projection_exec` break downstream builds? Since you mentioned this
pr is only additive.
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -444,16 +456,20 @@ pub trait PhysicalPlanNodeExt: Sized {
let plan_clone = Arc::clone(&plan);
let plan = plan.as_ref();
- if let Some(exec) = plan.downcast_ref::<ExplainExec>() {
- return protobuf::PhysicalPlanNode::try_from_explain_exec(exec,
codec);
+ // Self-serializing plans handle themselves via the `try_to_proto` hook
+ // (#22419). `Ok(None)` means "not migrated" and falls through to the
+ // central downcast chain below.
+ let encoder = ConverterPlanEncoder {
+ codec,
+ proto_converter,
+ };
+ let encode_ctx = ExecutionPlanEncodeCtx::new(&encoder);
+ if let Some(node) = plan.try_to_proto(&encode_ctx)? {
Review Comment:
Could we follow `downcast_delegate()` before calling this hook? A wrapper
that delegates to `ProjectionExec` calls the default hook on the wrapper and
gets `None`. Since the old projection fallback is removed, serialization fails.
--
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]
