metesynnada commented on PR #9236: URL: https://github.com/apache/arrow-datafusion/pull/9236#issuecomment-1961474987
No worries :) For `try_embed_to_hash_join` 1. **Comments and Documentation**: The initial comments are helpful, but they could be more detailed, especially in explaining the purpose and workings of the function. Include information about the function's parameters, return type, and a more detailed explanation of the logic within the function. Documenting each step within the function would also make it more readable and maintainable. 2. **Projection Length Check**: The check **`projection_index.len() >= hash_join.schema().fields().len()`** is slightly unclear. A comment explaining the logic behind this check would be beneficial. For instance, if the intent is to verify that the projection doesn't have more fields than the hash join, explaining why this condition leads to an early return would be helpful. 3. **Improve Projection Removability Logic:** The final decision to use **`new_hash_join`** or **`new_projection`** is based on whether the projection is removable. It would be helpful to add comments or documentation on how **`is_projection_removable`** works and under what conditions it considers a projection removable. -- 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]
