grundprinzip commented on PR #41328: URL: https://github.com/apache/spark/pull/41328#issuecomment-1569575227
@beliefer First of all, thanks a lot for the proposal and the approach to improving the quality and readability of the Spark Connect code. Personally, I don't think this change is a good approach for the following reasons: * The logic in `checkRelation` essentially becomes a shadow path to the logic in the actual planner. For the reviewer of the code and the writer, this means they have to validate the code and the behavior in two places and make sure that the behaviors are congruent. * In addition, when reviewing these changes of the refactoring, it is already hard now to reason about the probability of regression because the refactoring is not just purely cosmetic but for example, changes the actual branches and conditions of the planning logic. I agree that some of the removed branches are unreachable after the checker is introduced but I wanted to raise the complication of the review itself. * Lastly, I think that this separation in general will reduce the reviewability of the code because the refactoring extracts non-trivial logical analysis that is easier understood close to where it's actually evaluated. That said, I think we definitely have room for improvement to increase the readability and style of the code to make it easier to contribute. Maybe there are slightly smaller and more incremental ways to improve it? Thanks again for trying the approach! -- 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]
