ozankabak commented on PR #5322: URL: https://github.com/apache/arrow-datafusion/pull/5322#issuecomment-1437363201
> Something that may also help would be to break this PR into several smaller PRs if possible (I haven't looked at the code yet so I don't know how easy / hard this would be to do). It may be possible to do this if we carve out things like interval arithmetic, constraint propagation etc. into separate PRs. However, that will deprive you of important context (i.e. where/how do we use these things and why do they have the APIs that they have). For this reason, I suggest keeping this in one piece, and following a top-down order to make reviewing easier (and in parts): - Starting with the join-related code, it is quite similar to already existing join code and you will see the touch points of lower-level code. This is probably a good break point (e.g. can context switch to something else). - Looking at the code dealing with expression DAGs, how they are built & processed, where you will see the touch points of things like interval arithmetic. Again this is potentially a good break point too. - Lowest level code such as interval arithmetic > The rationale for more smaller PRs is that they require less contiguous time to review. For me, at least, finding 10 minutes to review a small PR is much easier to find 2 hours to review a larger one. Other reviewers may have a different opinion, of course, but that is mine I agree, it comes down to personal work style. Let's play by ear and see if my suggestions above help. If not, we can always try breaking it up. Our goal is to make the review process be efficient/easy, after all 🙂 -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
