2010YOUY01 commented on PR #25363: URL: https://github.com/apache/datafusion/pull/25363#issuecomment-6183169314
Thank you, this optimization is a neat idea. However, I'm not sure if it's a good performance v.s. maintenance tradeoff. The issue is this optimization has large overlap with the existing `ArrayMap` optmization. `ArrayMap` targets a very similar workload: if the HJ key is integer-only and has a small range (e.g. the build side has a key range of [1, 100]), it directly uses an array instead of a hash map as the index. The tradeoff is: - **ArrayMap:** Faster, more memory - **This PR:** Slower, less memory However, the typical workload targeted by this type of optimization has a very small build-side key range. I'm not convinced we need a more memory-efficient variant to fill this gap. Also, since this is a pre-filter rather than a mechanism that skips work entirely, the potential improvement is smaller. The existing dynamic filter can also cover this workload to some extent. My overall judgment is that we should revert this change, as the benefits don't seem to justify the additional maintenance overhead (four new configuration options are hard to tune, and having two closely related optimizations makes the execution path harder to predict). Would love to hear your thoughts, @lyne7-sc. -- 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]
