adriangb commented on PR #19538: URL: https://github.com/apache/datafusion/pull/19538#issuecomment-3819846616
> > I meant 700 LOC more than this PR. But yes it’s a lot to review. I can split it up, but it will be harder to see the big picture. Eg if I sequence (1) Expr APIs, (2) logical optimizer and (3) physical optimizer they will each be ~500 LOC each and easier to review, but are you okay making the Expr/PhysicalExpr change with the justification of “optimizer rules that use this coming later”? I can add unit tests, etc to show that those APIs work as intended. > > I think breaking up the PR into multiple smaller step PR, each linking to this final one for the context that show how it all fits together, works pretty well (though it is harder for the author) > > > I’m also happy to tap some other folks to help review. I know there are a lot of people interested in this work. > > That would be really good I think, not just for me to avoid being a bottleneck but to get some more real potential future users of this API to offer guidance too. Perhaps @AdamGS or @blaginin from Vortex have some time (or know of others from the Vortex world that can help) 100% agreed. I'm in the process of updating the first post in https://github.com/apache/datafusion/issues/19387 with a summary of the state of things. And I've started breaking out this PR: https://github.com/apache/datafusion/pull/20065 -- 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]
