[GitHub] [arrow] vibhatha commented on pull request #13401: ARROW-16855: [C++] Adding Read Relation ToProto

[GitBox <gi...@apache.org>]

vibhatha commented on PR #13401:
URL: https://github.com/apache/arrow/pull/13401#issuecomment-1205452548

   > Thanks for this work! It'll be very useful in testing to have roundtrippable full plans. I do have some concerns wrt the large-scale structure of this patch, though:
   > 
   > Maybe I've missed something, but I don't see the necessity of wrapping this in a registry. We have a number of registries in arrow but it's a pattern with heavy overhead which we follow only when required by constraints of third party extensibility. In this case, since the protobuf message classes are a private/internal implementation detail, adding anything to this registry would require rebuilding arrow anyway. That being the case, please remove the registry and ensure that protobuf message classes remain internal.
   
   Thank you @bkietz for the detailed review. I was inclined towards the registry with the idea of putting the Substrait-to-Acero and Acero-to-Substrait to be unified under a single registry. Not sure if it is a good idea. And this idea is not reflected in this PR. 
   
   We can easily move for a non-registry approach it is no issue. 
   
   cc @westonpace for more thoughts on this. 


-- 
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@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org