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

Jump to bottom

Support selecting different hash functions in hash_partition #6726

Merged
merged 9 commits into from
Dec 3, 2020
Merged

Support selecting different hash functions in hash_partition #6726

Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 1 addition & 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 @@ -16,20 +16,10 @@
#pragma once

#include <cudf/hashing.hpp>
#include <cudf/types.hpp>

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::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource(),
cudaStream_t stream = 0);

/**
* @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
4 changes: 3 additions & 1 deletion cpp/include/cudf/partitioning.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ 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::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());
hash_id hash_function = hash_id::HASH_MURMUR3,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource(),
cudaStream_t stream = 0);

/**
* @brief Round-robin partition.
Expand Down
Loading