-
Notifications
You must be signed in to change notification settings - Fork 912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor histogram
reduction using cuco::static_set::insert_and_find
#16485
Refactor histogram
reduction using cuco::static_set::insert_and_find
#16485
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
histogram
reduction using cuco::static_map::insert_or_apply
histogram
reduction using cuco::static_set::insert_and_find
@@ -1,169 +0,0 @@ | |||
/* | |||
* Copyright (c) 2022-2024, NVIDIA CORPORATION. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer in use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Requesting @ttnghia's review as he worked on the original hash reduce-by-row implementation.
// Hard set the tparam `has_nested_columns` = false for now as we don't yet support nested columns | ||
auto const key_equal = row_comp.equal_to<false>(has_nulls, null_equality::EQUAL, value_comp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is error prone. Suppose that now we want to support the nested types, it will be difficult to trace back to this line of code to change it. So I would rather implement the code path ready for nested types here, like what it was before:
if (has_nested_columns) {
auto const key_equal = row_comp.equal_to<true>(has_nulls, null_equality::EQUAL, value_comp);
map.insert(pair_iter, pair_iter + input.num_rows(), key_hasher, key_equal, stream.value());
} else {
auto const key_equal = row_comp.equal_to<false>(has_nulls, null_equality::EQUAL, value_comp);
map.insert(pair_iter, pair_iter + input.num_rows(), key_hasher, key_equal, stream.value());
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about the implicit use of non-nested key equality. Instead of adding a code path for nested types that isn't used by any existing code, I suggest leaving the code as is and adding a CUDF_EXPECTS
check to ensure no nested types are involved in the calculation. This approach also avoids templating functions, which can help reduce build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The performance improvement is fantastic. There's just one small request.
Reverting the added functor which enabled the unused code path in light of Yunsong's comment. |
/merge |
Description
Refactors
histogram
reduce and groupby aggregations usingcuco::static_set::insert_and_find
. Speed improvement results here and here.Checklist