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.

Sign up for GitHub

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

Support selecting different hash functions in hash_partition #6726

Merged
merged 9 commits into from
Dec 3, 2020
+85 −621
Merged

Support selecting different hash functions in hash_partition #6726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6bb99f0
Select hash functions in hash_partition
gaohao95 Nov 10, 2020
e5b380a
Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into s…
gaohao95 Nov 11, 2020
f9fcc29
Throw hash partition error on the host
gaohao95 Nov 11, 2020
461049c
Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into s…
gaohao95 Dec 1, 2020
89bcabc
Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into s…
gaohao95 Dec 2, 2020
a7acd21
CHANGELOG
gaohao95 Dec 2, 2020
effa95d
Merge branch 'branch-0.17' of https://github.com/rapidsai/cudf into s…
gaohao95 Dec 3, 2020
a2571ba
Add missing header
gaohao95 Dec 3, 2020
bac6457
Group headers by library
gaohao95 Dec 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
- PR #6765 Cupy fallback for __array_function__ and __array_ufunc__ for cudf.Series
- PR #6817 Add support for scatter() on lists-of-struct columns
- PR #6805 Implement `cudf::detail::copy_if` for `decimal32` and `decimal64`
- PR #6726 Support selecting different hash functions in hash_partition
- PR #6619 Improve Dockerfile

## Improvements
Expand Down
11 changes: 0 additions & 11 deletions cpp/include/cudf/detail/hashing.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,6 @@

namespace cudf {
namespace detail {
/**
* @copydoc cudf::hash_partition
*
* @param stream CUDA stream used for device memory operations and kernel launches.
*/
std::pair<std::unique_ptr<table>, std::vector<size_type>> hash_partition(
table_view const& input,
std::vector<size_type> const& columns_to_hash,
int num_partitions,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
* @copydoc cudf::hash
Expand Down
13 changes: 12 additions & 1 deletion cpp/include/cudf/detail/utilities/hash_functions.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#pragma once

#include <cudf/column/column_device_view.cuh>
#include <cudf/detail/utilities/release_assert.cuh>
#include <cudf/strings/string_view.cuh>
#include <hash/hash_constants.hpp>

Expand Down Expand Up @@ -578,7 +579,17 @@ struct IdentityHash {
return combined;
}

CUDA_HOST_DEVICE_CALLABLE result_type operator()(const Key& key) const
template <typename return_type = result_type>
CUDA_HOST_DEVICE_CALLABLE std::enable_if_t<!std::is_arithmetic<Key>::value, return_type>
operator()(const Key& key) const
{
release_assert(false && "IdentityHash does not support this data type");
return 0;
}
Comment on lines +582 to +588
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? I think that this code turns a compile time error into a runtime error, which is not good for developers because it will cause them to find their coding errors later.

If you remove this, does the code still compile? If so then this code is never generating anything so it can be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function object is invoked via the type_dispatcher which will instantiate it for all possible libcudf types. We need to provide a valid instantiation for all types. This includes types that should never actually be invoked (as seen above).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so if these lines of code are removed then cudf will no longer compile successfully?

Copy link
Contributor Author

@gaohao95 gaohao95 Dec 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case yes. static_cast a string to an integer should fail at compile time.


template <typename return_type = result_type>
CUDA_HOST_DEVICE_CALLABLE std::enable_if_t<std::is_arithmetic<Key>::value, return_type>
operator()(const Key& key) const
{
return static_cast<result_type>(key);
}
Expand Down
5 changes: 5 additions & 0 deletions cpp/include/cudf/partitioning.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
#pragma once

#include <cudf/types.hpp>

#include <rmm/cuda_stream_view.hpp>

#include <memory>
#include <vector>

Expand Down Expand Up @@ -88,6 +91,8 @@ std::pair<std::unique_ptr<table>, std::vector<size_type>> hash_partition(
table_view const& input,
std::vector<size_type> const& columns_to_hash,
int num_partitions,
hash_id hash_function = hash_id::HASH_MURMUR3,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
gaohao95 marked this conversation as resolved.
Show resolved Hide resolved
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

/**
Expand Down
Loading