Repository navigation
[ENHANCEMENT]: Consider removing the use of argument_type and result_type from hasher in cuco default_filter_policy #653
Description
Is your feature request related to a problem? Please describe.
The member type std::hash::argument_type is deprecated in C++17 and removed in C++20, so we should consider syncing with STL and remove them from our hashers as well as their use in bloom filter policies.
Ref: https://en.cppreference.com/w/cpp/utility/hash
Originally posted by @bdice in NVIDIA/cudf#17289 (comment)
Describe the solution you'd like
Consider refactoring to not use hasher::argument_type and hasher::result_type across cuCollections.
Describe alternatives you've considered
Keep using the deprecated hasher type aliases until C++20
Additional context
No response
Activity
[-][ENHANCEMENT]: Consider removing the use of `hasher::argument_type` and `hasher::result_type` in cuCo default and arrow filter policies[/-][+][ENHANCEMENT]: Consider removing the use of `argument_type` and `result_type` from `hasher` in cuo default and arrow filter policies[/+]PointKernel commentedon Dec 10, 2024
I thought about mentioning this during the code review but didn’t want to come across as nitpicking. Instead of
cuCollections/include/cuco/detail/bloom_filter/arrow_filter_policy.cuh
Lines 84 to 85 in b8429d4
we could make the hasher a template type:
To use it, users can simply pass the hasher type without repeating the
Keytype again, e.g.auto p = arrow_filter_policy<Key, cuco::xxhash_32>{...};mhaseeb123 commentedon Dec 10, 2024
No worries about that. I am always for pursuing excellence and attention to the detail is how we all learn. 😄
I think this is a great idea. Let's do this. But we still would be using the member types (
argument_typeandresult_type) from the hasher which Bradley mentionedPointKernel commentedon Dec 10, 2024
Half-half. The
argument_typewill simply bekey_type (using key_type = Key). However, retainingresult_type(should behash_value_typeof the policy) is still desirable. The STL simplifies hashers by always usingstd::size_tas the return type, but this approach is suboptimal, especially in terms of performance and for GPUs. Such oversimplification does not fit our goals and IMO should be avoided.mhaseeb123 commentedon Dec 10, 2024
Agreed. Let's make the suggested change to
cuco::arrow_filter_policyand close this issue otherwise.[-][ENHANCEMENT]: Consider removing the use of `argument_type` and `result_type` from `hasher` in cuo default and arrow filter policies[/-][+][ENHANCEMENT]: Consider removing the use of `argument_type` and `result_type` from `hasher` in cuco `default_filter_policy`[/+]mhaseeb123 commentedon Dec 10, 2024
#654 removes these member types for the
arrow_filter_policy.