Re: [I] Allow extension `ExecutionPlan`s to decode via a per-type registry instead of `PhysicalExtensionCodec` [datafusion]

["gabotechs (via GitHub)" <gi...@apache.org>]

gabotechs commented on issue #24625:
URL: https://github.com/apache/datafusion/issues/24625#issuecomment-5572523981

   Just sumarizing here the API proposed in https://github.com/apache/datafusion/pull/24631:
   
   ## Before
   
   This is how today users can register their own codecs, and do a roundtrip over a custom plan.
   
   ```rust
   struct MyExec;
   
   struct MyExecCodec;
   
   impl PhysicalExtensionCodec for MyExecCodec {
       fn try_encode(
           &self,
           node: Arc<dyn ExecutionPlan>,
           buf: &mut Vec<u8>,
           proto_converter: &dyn PhysicalProtoConverterExtension,
       ) -> Result<()> {
           todo!() // encode logic
       }
   
       fn try_decode(
           &self,
           buf: &[u8],
           inputs: &[Arc<dyn ExecutionPlan>],
           ctx: &TaskContext,
           proto_converter: &dyn PhysicalProtoConverterExtension,
       ) -> Result<Arc<dyn ExecutionPlan>> {
           todo!() // decode logic
       }
   }
   
   fn main() {
       let session_config = SessionConfig::new();
       let session_state = SessionStateBuilder::new()
           .with_config(session_config)
           .build();
       let task_ctx = session_state.task_ctx();
   
       let plan = Arc::new(MyExec);
   
       let codec = ComposedPhysicalExtensionCodec::new(vec![Arc::new(MyExecCodec)]);
   
       let proto = PhysicalPlanNode::try_from_physical_plan_with_converter(
           plan,
           &codec,
           &DefaultPhysicalProtoConverter {},
       )?;
       let plan = proto.try_into_physical_plan_with_context(
           &PhysicalPlanDecodeContext::new(&task_ctx, &codec),
           &DefaultPhysicalProtoConverter {},
       )?;
   }
   ```
   
   ## After
   
   This is the alternative based on a type registry based on the current state of https://github.com/apache/datafusion/pull/24631:
   
   ```rust
   struct MyExec;
   
   impl ExecutionPlan for MyExec {
       fn try_to_proto(
           &self,
           ctx: &ExecutionPlanEncodeCtx<'_>,
       ) -> Result<Option<PhysicalPlanNode>> {
           todo!() // encode logic
       }
   }
   
   impl ExtensionExecutionPlan for MyExec {
       const PLAN_NAME: &'static str = "my-crate.MyExec";
   
       fn try_from_proto(
           node: &PhysicalPlanNode,
           ctx: &ExecutionPlanDecodeCtx<'_>,
       ) -> Result<Arc<dyn ExecutionPlan>> {
           todo!() // decode logic
       }
   }
   
   fn main() {
       let session_config = SessionConfig::new().with_execution_plan::<MyExec>()?;
       let session_state = SessionStateBuilder::new()
           .with_config(session_config)
           .build();
       let task_ctx = session_state.task_ctx();
   
       let plan = Arc::new(MyExec);
   
       let proto = PhysicalPlanNode::try_from_physical_plan_with_converter(
           plan,
           &DefaultPhysicalExtensionCodec {},
           &DefaultPhysicalProtoConverter {},
       )?;
       let plan = proto.try_into_physical_plan_with_context(
           &PhysicalPlanDecodeContext::new(&task_ctx, &DefaultPhysicalExtensionCodec {}),
           &DefaultPhysicalProtoConverter {},
       )?;
   }
   ```
   
   Here are a couple of thoughts:
   - The type-registry based API is slightly nicer to work with, although the impact in the API change does not seem very big. The most important thing probably is that it would allow to consumers to remove big downcast-branch functions into per-plan scoped implementations.
   - The new API seems to be coupled to protobuf, but the `PhysicalExtensionCodec` allows plain bytes, which opens the door for arbitrary encodings. In my selfish opinion this is not a big deal, but I wonder if there are people in the community relying on non-protobuf based encodings.
   - The concurrent presence of the two APIs for providing serde capabilities to plans (`PhysicalExtensionCodec` + type registry) is probably not very nice from a public API standpoint. My guess is that it'd make sense to deprecate `PhysicalExtensionCodec` if we want to pursue the type registry based API.
   
   As a user of the `PhysicalExtensionCodec` + `ComposedPhysicalExtensionCodec`, I have to admit that it's actually not that bad, and it gets the job done with no measurable performance impact. I'd even say that a successor of `ComposedPhysicalExtensionCodec` that stores a registry of types rather than a vec of codecs to try sequentially has good options for bringing most of the benefits of this proposed API with a very scoped change.


-- 
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