Refactor semi_anti_join#11100
Merged
rapids-bot[bot] merged 63 commits intorapidsai:branch-22.08from Jul 14, 2022
Merged
Conversation
ttnghia
added
feature request
ttnghia
changed the title
semi_anti_join to expose semi_join_containssemi_anti_join to expose left_semi_join_contains
Contributor
Author
|
Benchmark comparing before and after the modification: So the performance is not always better, although in most cases we gain >10% speedup. The slower places are due to null search. I'm still trying to optimize it. |
ttnghia
added
3 - Ready for Review
ttnghia
removed
0 - Blocked
PointKernel
approved these changes
Jul 13, 2022
Member
PointKernel
left a comment
PointKernel
left a comment
There was a problem hiding this comment.
Some non-blocking nitpick. Thanks @ttnghia ! Great Work!
cpp/src/search/contains_table.cu
Outdated
| // Otherwise, we have only one nullable column and can use its null mask directly. | ||
| auto const row_bitmask = | ||
| haystack_nullable_columns.size() > 1 | ||
| ? std::move( |
Contributor
Author
There was a problem hiding this comment.
move is necessary here because we need to access to the .first element of the bitmask_and result.
Member
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment.
Wow I didn't know that. Thanks 😄
Contributor
Author
There was a problem hiding this comment.
Sorry, I was wrong, the .first element here is a device_buffer instead.
It seems that by not using std::move, the device_buffer is incorrectly copied (??) and CI test failed:
13:43:43 [ RUN ] DeathTest.CudaFatalError
13:43:43 /workspace/.conda-bld/work/cpp/tests/error/error_handling_test.cu:102: Failure
13:43:43 Death test: call_kernel()
13:43:43 Result: failed to die.
13:43:43 Error msg:
13:43:43 [ DEATH ]
Contributor
Author
There was a problem hiding this comment.
Let me try one more time to see if removing std::move actually can get rid of the death test failure.
Contributor
Author
There was a problem hiding this comment.
Maybe the failure here is just random? I just checked rmm::device_buffer and see that its copy constructor is deleted so there should not be any copying.
Let's wait to see the CI test result.
Contributor
Author
There was a problem hiding this comment.
Okay that test failed again. So indeed removing std::move is not related.
Contributor
Author
There was a problem hiding this comment.
Now all CI tests passed (std::move removed)! Definitely the test failed randomly. I'm going to merge this soon.
Contributor
Author
|
Rerun tests. |
ttnghia
dismissed
jrhemstad’s stale review
Unfortunately Jake has no time to review again
Contributor
Author
|
@gpucibot merge |
This was referenced Jul 20, 2022
rapids-bot bot
pushed a commit
that referenced
this pull request
Jul 26, 2022
This PR adds the following APIs for set operations: * `lists::have_overlap` * `lists::intersect_distinct` * `lists::union_distinct` * `lists::difference_distinct` ### Name Convention Except for the first API (`lists::have_overlap`) that returns a boolean column, the suffix `_distinct` of the rest APIs denotes that their results will be lists columns in which all list rows have been post-processed to remove duplicates. As such, their results are actually "set" columns in which each row is a "set" of distinct elements. --- Depends on: * #10945 * #11017 * NVIDIA/cuCollections#175 * #11052 * #11118 * #11100 * #11149 Closes #10409. Authors: - Nghia Truong (https://github.com/ttnghia) - Yunsong Wang (https://github.com/PointKernel) Approvers: - Michael Wang (https://github.com/isVoid) - AJ Schmidt (https://github.com/ajschmidt8) - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) URL: #11043
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A (left) semi-join between the left and right tables returns a set of rows in the left table that has matching rows (i.e., compared equally) in the right table. As such, for each row in the left table, it needs to check if that row has a match in the right table.
Such check is very generic and has applications in many other places, not just in semi-join. This PR exposes that check functionality as a new
cudf::detail::contains(table_view, table_view)for internal usage.Closes #11037.
Depends on:
static_multimap::pair_containsNVIDIA/cuCollections#175