Refactor cudf::detail::contains(table_view, table_view)
#11325
Closed
Conversation
This file contains 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
ttnghia
added
3 - Ready for Review
Codecov Report
@@ Coverage Diff @@
## branch-22.08 #11325 +/- ##
===============================================
Coverage ? 86.39%
===============================================
Files ? 143
Lines ? 22753
Branches ? 0
===============================================
Hits ? 19658
Misses ? 3095
Partials ? 0 Continue to review full report at Codecov.
|
PointKernel
approved these changes
Jul 21, 2022
| template <typename T> | ||
| __device__ inline auto operator()(T const idx) const noexcept | ||
| { | ||
| return _hasher(static_cast<size_type>(idx)); |
There was a problem hiding this comment.
Will there be narrow conversion issues?
There was a problem hiding this comment.
This functor is only used locally in this file, and the input idx is only strong index types (either lhs_index_type or rhs_index_type).
|
This was looking good to me. I'd like to get my head around the null-checking before I approve. |
|
Do we have any performance results for this yet? |
|
Close this as it is covered in a new PR: #11330. |
rapids-bot bot
pushed a commit
that referenced
this pull request
Jul 22, 2022
…ins` (#11330) The current implementation of `cudf::detail::contains` can process input with arbitrary nested types. However, it was reported to have severe performance issue when the input tables have many duplicate rows (#11299). In order to fix the issue, #11310 and #11325 was created. Unfortunately, #11310 is separating semi-anti-join from `cudf::detail::contains`, causing duplicate implementation. On the other hand, #11325 can address the issue #11299 but semi-anti-join using it still performs worse than the previous semi-anti-join implementation. The changes in this PR include the following: * Fix the performance issue reported in #11299 for the current `cudf::detail::contains` implementation that support nested types. * Add a separate code path into `cudf::detail::contains` such that: * Input without having lists column (at any nested level) will be processed by the code path that is the same as the old implementation of semi-anti-join. This is to make sure the performance of semi-anti-join will remain the same as before. * Input with nested lists column, or NaNs compared as unequal, will be processed by another code path that supports nested types and different NaNs behavior. This will make sure support for nested types will not be dropped. Closes #11299. Authors: - Nghia Truong (https://github.com/ttnghia) Approvers: - Yunsong Wang (https://github.com/PointKernel) - Bradley Dice (https://github.com/bdice) - MithunR (https://github.com/mythrocks) - Vyas Ramasubramani (https://github.com/vyasr) - Mike Wilson (https://github.com/hyperbolic2346) - Alessandro Bellina (https://github.com/abellina) URL: #11330
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR modifies the overload of
cudf::detail::containsthat accepts a pair oftable_view. The main change here is switching to usingcuco::static_mapinstead ofcuco::static_multimap. This can avoid the performance regression due to the limitation of using multimap as has been reported in #11299, when the input tables have many duplicate rows.