gianm commented on PR #13506: URL: https://github.com/apache/druid/pull/13506#issuecomment-1439929888
> Okay, I've now read through all of the classes. Or, read them as best I can. I'm a newbie to this code, so the best I can do is a surface-level review, which is what I've done. Most of my comments are stream-of-consciousness because, I fear that I lack the context to really comment meaningfully. Thanks, I still appreciate it. > In terms of approving merge or not approving merge. I don't know how "safe" this is. It seems to touch a lot of different files and there does seem to be good test coverage, is this safe to just merge and it'll work? Is there anything extra done in the code to ensure that the old code paths are untouched in the worst case that there is a bug in here? The SQL planning changes are designed to be low risk when not using sort-merge joins. The new code is generally additive and the bulk of it will not execute for the default (broadcast) join algorithm. The MSQ and processing changes, other than the core sort-merge logic, are mostly related to adding hash partitioning. I tried to keep this as minimally invasive as possible, but it did have to touch various places and it could not be purely additive. To minimize the risk, I've added new tests. I will fix any problems that turn up. -- 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]
