Refactor distinct hash join to handle multiple probes with the same build table #17609
Conversation
PointKernel
added
feature request
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
| @@ -469,20 +461,19 @@ class hash_join { | |||
| rmm::device_async_resource_ref mr = cudf::get_current_device_resource_ref()) const; | |||
|
|
|||
| private: | |||
| const std::unique_ptr<impl_type const> _impl; | |||
| std::unique_ptr<impl_type const> _impl; | |||
There was a problem hiding this comment.
Avoid const data member per https://quuxplusone.github.io/blog/2022/01/23/dont-const-all-the-things/#data-members-never-const
PointKernel
added
3 - Ready for Review
That makes sense. I have it on my radar: #13700 |
|
Note that this will require some meaningful refactoring of the implementation. |
| cudf::detail::cuco_allocator<char>, | ||
| cuco_storage_type>; | ||
|
|
||
| bool _has_nulls; ///< True if nulls are present in either build table or probe table |
There was a problem hiding this comment.
Since we do not specify the probe table, and we may not know anything about the probe tables in the future, should we set this always be true? Otherwise, specifying false but the probe table has nulls then the result may be incorrect.
I remember that we've dealt with this same issue before, with some previous version of this join code.
There was a problem hiding this comment.
I'm following the same null check pattern used in hash_join, so I assume downstream users like Spark are handling this correctly. If needed, I'm happy to open a separate PR to update both distinct_hash_join and hash_join to improve this.
There was a problem hiding this comment.
Oh actually you reviewed that PR. Here it is: #13120.
The bug was fixed by checking hash_nulls on both table, like in https://github.com/rapidsai/cudf/pull/13120/files#diff-734dd746efe774e94501b6aa986e7824656182f85bf9af8bf30036c002ca1b82R82.
There was a problem hiding this comment.
I'm not sure if Spark can handle this correctly but assuming that we stumbled upon the same issue before, I would highly suspect that it couldn't do it.
There was a problem hiding this comment.
I think Spark handles this properly, e.g.:
cudf/java/src/main/native/src/TableJni.cpp
Lines 2904 to 2906 in e8935b9
Are you suggesting that we should remove _has_null as a data member as it's always true?
There was a problem hiding this comment.
Yes, I think so. Previously we have both tables thus we can compute has_nulls but now we only have one table thus this variable should be removed and the corresponding nullate value should always be true.
cpp/src/join/distinct_hash_join.cu
Outdated
| auto const has_nulls = _has_nulls or cudf::has_nested_nulls(probe); | ||
| auto const d_probe_hasher = probe_row_hasher.device_hasher(nullate::DYNAMIC{has_nulls}); |
There was a problem hiding this comment.
I still see problem here. When has_nulls is true due to nulls in the probe table but _has_nulls is false, the build table was hashed by a different way than the probe table. As such, two identical rows in two tables may be hashed to different values, which lead to incorrect join output.
There was a problem hiding this comment.
I finally get what you mean. Should we do the same cleanup for hash_join as well?
There was a problem hiding this comment.
Oh so the same issue was already merged before? I didn't know that (or maybe don't remember). Yes, I think this is a real bug and should be addressed completely. If that issue was already merged before and is not related to this PR, we can fix it in a separate work.
There was a problem hiding this comment.
distinct hash join is fixed in this PR. I will address hash_join separately.
|
/merge |
rapids-bot
bot
merged commit 62d72df
into
rapidsai:branch-25.02
Description
This PR updates the distinct join implementation to allow the same build table to be reused for multiple probe operations. It also introduces several breaking changes, including removing the need for users to specify whether the input data contains nested columns. Additionally, the output order has been updated to align with the hash join behavior, with probe indices now appearing on the left and build indices on the right.
The PR leverages the new conditional query API in the cuco hash set, enabling more efficient handling of nullable data. While this optimization improves performance, it is not currently reflected in benchmarks due to the absence of a dedicated test case for this scenario.
Checklist