2010YOUY01 commented on PR #23870: URL: https://github.com/apache/datafusion/pull/23870#issuecomment-5224138428
Thank you for this really nice PR, I got some suggestions: - (optional) Split the implementation for semi/anti to a new stream: although the high-level algorithm is similar, semi/anti and regular joins are still different relational operator, and have different algorithm/optimization details, separating them can make the code easier to maintain. (sorry I only figured this out after writing the original suggestion in https://github.com/apache/datafusion/pull/18392#issuecomment-4277614451) If you opt to leave it to a follow-up, we might need some renaming/doc updates to avoid confusion. The existing term `classic_join` means non semi/anti joins, and now we're implementing semi/anti joins inside `classic_join.rs` - Move most UTs to sqllogictests: as sqls are easier to maintain and stronger, they can also also exercise potential related optimizations like swap joins. I think for UT only basic/demo tests are needed. This high-level approach LGTM, I might not be able to do detailed follow-up review timely, but it should be good to go if others can do the review. -- 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]
