Skip to content
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

[FEATURE]: Expose hash_function member function for hash tables #582

Closed
PointKernel opened this issue Aug 20, 2024 · 3 comments · Fixed by #587
Closed

[FEATURE]: Expose hash_function member function for hash tables #582

PointKernel opened this issue Aug 20, 2024 · 3 comments · Fixed by #587
Labels
helps: rapids Helps or needed by RAPIDS P1: Should have Necessary but not critical type: feature request New feature request

Comments

@PointKernel
Copy link
Member

PointKernel commented Aug 20, 2024

Is your feature request related to a problem? Please describe.

The current cuco hash tables don't have hash_function as a member function because we didn't come up with a proper solution for double hashing, i.e., the underlying hash table may utilize more than one hash function so the below interface doesn't seem to be right:

hasher hash_function () const { ... }

Describe the solution you'd like

However, the original idea is not bad and we could use cuda::std::pair<Hash1, Hash2> to define hasher in the case of double hashing:

class static_map {
  ...
  using hasher = cuda::std::conditional_t<is_double_hashing<probing_scheme_type>::value,
    cuda::std::pair<probing_scheme_type::Hash1, probing_scheme_type::Hash2>,
    probing_scheme_type::Hash>;

  ...
  hasher constexpr hash_function () const {
    return cuda::std::pair{probing_scheme.hash_function()};
  }
};

In the future, we can also switch to cuda::std::tuple if more than two hashers are needed.

Describe alternatives you've considered

No response

Additional context

We need a build-time is_double_hashing trait to implement this.

@PointKernel PointKernel added type: feature request New feature request helps: rapids Helps or needed by RAPIDS P1: Should have Necessary but not critical labels Aug 20, 2024
@sleeepyjack
Copy link
Collaborator

In the future, we can also switch to cuda::std::tuple if more than two hashers are needed.

I would go with cuda::std::tuple from the start. The only difference is that pair provides .first and .second members. A tuple is more generic and requires no future code change.

PointKernel added a commit that referenced this issue Aug 21, 2024
This PR fixes a small bug that the current `probing_scheme.cuh` header
is not self-contained due to a missing inclusion. It also adds a trait
to help #581 and #582.
@sleeepyjack
Copy link
Collaborator

Shall we move this logic to the probing_scheme class? double_hashing::hasher would define tuple<Hash1, Hash2> while the other probing scheme types would just use Hash. From there: auto static_map::hash_function() { probing_scheme_.hash_function(); }

@PointKernel
Copy link
Member Author

Shall we move this logic to the probing_scheme class? double_hashing::hasher would define tuple<Hash1, Hash2> while the other probing scheme types would just use Hash. From there: auto static_map::hash_function() { probing_scheme_.hash_function(); }

Yes, that's what I have in mind as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helps: rapids Helps or needed by RAPIDS P1: Should have Necessary but not critical type: feature request New feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants