Skip to content

Commit

Permalink
Fix cudf::hash_partition kernel launch error with decimal128 types (#…
Browse files Browse the repository at this point in the history
…12863)

Fixes `cudf::hash_partition` error when using `decimal128` column types. The internal optimized path, `copy_block_partitions`, uses shared-memory for copying fixed-width type column elements. For `int128_t` type, the shared-memory needed (~64KB) is larger than the maximum size (~48KB) allowed causing a kernel launch failure. 

The optimized path is now restricted to only fixed-width types `int64_t` and below. The `int128_t` column types will fall through to the gather-map pattern instead. Accommodating this type in the existing copy-block implementation would likely penalize the performance of the other fixed-width types. If the new implementation becomes insufficient, we could explore a special optimized path in the future for the single type `int128_t`.

An existing gtest for fixed-point types was updated to include a `decimal128` column to catch this kind of error in the future.

Closes #12852

Authors:
  - David Wendt (https://github.com/davidwendt)
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Divye Gala (https://github.com/divyegala)
  - Bradley Dice (https://github.com/bdice)

URL: #12863
  • Loading branch information
davidwendt authored Mar 7, 2023
1 parent 0ee82c9 commit a78fdd5
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 14 deletions.
24 changes: 17 additions & 7 deletions cpp/src/partitioning/partitioning.cu
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

#include <cub/cub.cuh>
#include <cudf/column/column_factories.hpp>
#include <cudf/copying.hpp>
#include <cudf/detail/gather.hpp>
Expand All @@ -36,6 +35,9 @@
#include <thrust/scan.h>
#include <thrust/transform.h>

#include <cub/block/block_scan.cuh>
#include <cub/device/device_histogram.cuh>

namespace cudf {
namespace {
// Launch configuration for optimized hash partition
Expand Down Expand Up @@ -389,7 +391,15 @@ rmm::device_uvector<size_type> compute_gather_map(size_type num_rows,
}

struct copy_block_partitions_dispatcher {
template <typename DataType, std::enable_if_t<is_fixed_width<DataType>()>* = nullptr>
template <typename DataType>
constexpr static bool is_copy_block_supported()
{
// The shared-memory used for fixed-width types in the copy_block_partitions_impl function
// will be too large for any DataType greater than int64_t.
return is_fixed_width<DataType>() && (sizeof(DataType) <= sizeof(int64_t));
}

template <typename DataType, CUDF_ENABLE_IF(is_copy_block_supported<DataType>())>
std::unique_ptr<column> operator()(column_view const& input,
const size_type num_partitions,
size_type const* row_partition_numbers,
Expand All @@ -416,7 +426,7 @@ struct copy_block_partitions_dispatcher {
return std::make_unique<column>(input.type(), input.size(), std::move(output));
}

template <typename DataType, std::enable_if_t<not is_fixed_width<DataType>()>* = nullptr>
template <typename DataType, CUDF_ENABLE_IF(not is_copy_block_supported<DataType>())>
std::unique_ptr<column> operator()(column_view const& input,
const size_type num_partitions,
size_type const* row_partition_numbers,
Expand Down Expand Up @@ -713,7 +723,7 @@ struct dispatch_map_type {
} // namespace

namespace detail {
namespace local {
namespace {
template <template <typename> class hash_function>
std::pair<std::unique_ptr<table>, std::vector<size_type>> hash_partition(
table_view const& input,
Expand All @@ -738,7 +748,7 @@ std::pair<std::unique_ptr<table>, std::vector<size_type>> hash_partition(
input, table_to_hash, num_partitions, seed, stream, mr);
}
}
} // namespace local
} // namespace

std::pair<std::unique_ptr<table>, std::vector<size_type>> partition(
table_view const& t,
Expand Down Expand Up @@ -779,10 +789,10 @@ std::pair<std::unique_ptr<table>, std::vector<size_type>> hash_partition(
if (!is_numeric(input.column(column_id).type()))
CUDF_FAIL("IdentityHash does not support this data type");
}
return detail::local::hash_partition<detail::IdentityHash>(
return detail::hash_partition<detail::IdentityHash>(
input, columns_to_hash, num_partitions, seed, stream, mr);
case (hash_id::HASH_MURMUR3):
return detail::local::hash_partition<detail::MurmurHash3_32>(
return detail::hash_partition<detail::MurmurHash3_32>(
input, columns_to_hash, num_partitions, seed, stream, mr);
default: CUDF_FAIL("Unsupported hash function in hash_partition");
}
Expand Down
14 changes: 7 additions & 7 deletions cpp/tests/partitioning/hash_partition_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -349,21 +349,21 @@ TEST_F(HashPartition, FixedPointColumnsToHash)
{
fixed_width_column_wrapper<int32_t> to_hash({1});
cudf::test::fixed_point_column_wrapper<int64_t> first_col({7}, numeric::scale_type{-1});
cudf::test::fixed_point_column_wrapper<__int128_t> second_col({77}, numeric::scale_type{0});

auto first_input = cudf::table_view({to_hash, first_col});
auto input = cudf::table_view({to_hash, first_col, second_col});

auto columns_to_hash = std::vector<cudf::size_type>({0});

cudf::size_type const num_partitions = 1;
auto [first_result, first_offsets] =
cudf::hash_partition(first_input, columns_to_hash, num_partitions);
auto [result, offsets] = cudf::hash_partition(input, columns_to_hash, num_partitions);

// Expect offsets to be equal and num_partitions in length
EXPECT_EQ(static_cast<size_t>(num_partitions), first_offsets.size());

CUDF_TEST_EXPECT_COLUMNS_EQUAL(first_result->get_column(0).view(), first_input.column(0));
EXPECT_EQ(static_cast<size_t>(num_partitions), offsets.size());

CUDF_TEST_EXPECT_COLUMNS_EQUAL(first_result->get_column(1).view(), first_input.column(1));
CUDF_TEST_EXPECT_COLUMNS_EQUAL(result->get_column(0).view(), input.column(0));
CUDF_TEST_EXPECT_COLUMNS_EQUAL(result->get_column(1).view(), input.column(1));
CUDF_TEST_EXPECT_COLUMNS_EQUAL(result->get_column(2).view(), input.column(2));
}

TEST_F(HashPartition, ListWithNulls)
Expand Down

0 comments on commit a78fdd5

Please sign in to comment.