timsaucer commented on PR #19437: URL: https://github.com/apache/datafusion/pull/19437#issuecomment-3699421831
> Sorry for late reply! Had a quick look, proposal does make sense to me. > > Introduction of `PhysicalExtensionProtoCodec` is a bit confusing to me, maybe we need better name. I believe average user does not need to know about it so `protobuf::PhysicalPlanNode::try_from_physical_plan` could stay as it was and we can add `protobuf::PhysicalPlanNode::try_from_physical_plan_with_custom_proto_codec(...)` (or similar) Sound advice! I wanted to make sure the approach as a whole seems reasonable to you both. I agree about the naming. It's not actually a codec. It doesn't actually do any serialization or deserialization. I tried working with a LLM to come up with a better name. - `PhysicalExtensionProtoAdapter` - `PhysicalExtensionProtoConverter` - `PhysicalExtensionProtoTranslator` And maybe the name ordering should actually be - `PhysicalProtoTranslatorExtension` because the thing we're extending is the proto translation? I'm on PTO through next week, but I think in principle we have a path we want to go down. I would suggest we give equal treatment to the logical side and make sure the current paths are not impacted/use the default. -- 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]
