timsaucer commented on PR #19267: URL: https://github.com/apache/datafusion/pull/19267#issuecomment-3667060145
This feels like it's more complicated than it needs to be. But I also must be missing something because I still don't see what it was that my original proposal in https://github.com/apache/datafusion/pull/18813 didn't meet the needs. I think you have some use cases and context in mind @adriangb that I clearly don't understand. Regarding this PR, do you also need to cover the other functions in the codec? We now have two methods we need for physical plans, `try_decode` and `deserialize_physical_plan`. The first seems very codec-y to me. Given a bunch of bytes, produce a plan. It doesn't even necessarily need to use protobuf (up to the implementer). The second seems to be more about hooking into the process of using the codecs + non-codecs. I think the problem we're running into with not having an elegant way of hooking these together is demonstrating there's a friction point between what the codec is for and what the protobuf processing part is for. I'm sure I'm biased towards my initial implementation approach, but I still feel that's a cleaner way of doing it. -- 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]
