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

Remove unused cudf::strings::create_offsets #8663

Merged
merged 6 commits into from
Jul 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 1 addition & 19 deletions cpp/include/cudf/strings/strings_column_view.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,8 @@
*/
#pragma once

#include <cudf/column/column.hpp>
#include <cudf/column/column_view.hpp>

#include <rmm/cuda_stream_view.hpp>
#include <rmm/device_uvector.hpp>

/**
* @file
* @brief Class definition for cudf::strings_column_view
Expand Down Expand Up @@ -86,21 +82,7 @@ class strings_column_view : private column_view {

//! Strings column APIs.
namespace strings {
/**
* @brief Create output per Arrow strings format.
*
* The return pair is the vector of chars and the vector of offsets.
*
* @param strings Strings instance for this operation.
* @param stream CUDA stream used for device memory operations and kernel launches.
* @param mr Device memory resource used to allocate the returned device vectors.
* @return Pair containing a vector of chars and a vector of offsets.
*/
std::pair<rmm::device_uvector<char>, rmm::device_uvector<size_type>> create_offsets(
strings_column_view const& strings,
rmm::cuda_stream_view stream = rmm::cuda_stream_default,
rmm::mr::device_memory_resource* mr = rmm::mr::get_current_device_resource());

} // namespace strings

/** @} */ // end of group
} // namespace cudf
1 change: 1 addition & 0 deletions cpp/include/cudf/table/table_device_view.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <cudf/types.hpp>

#include <rmm/cuda_stream_view.hpp>
#include <rmm/device_buffer.hpp>

#include <cassert>
#include <memory>
Expand Down
24 changes: 12 additions & 12 deletions cpp/include/cudf_test/column_utilities.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019-2020, NVIDIA CORPORATION.
* Copyright (c) 2019-2021, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -18,12 +18,13 @@

#include <cudf/column/column.hpp>
#include <cudf/column/column_view.hpp>
#include <cudf/detail/utilities/vector_factories.hpp>
#include <cudf/null_mask.hpp>
#include <cudf/strings/strings_column_view.hpp>
#include <cudf/types.hpp>
#include <cudf/utilities/error.hpp>
#include "cudf/utilities/traits.hpp"
#include "thrust/iterator/transform_iterator.h"

#include <thrust/iterator/transform_iterator.h>

namespace cudf {
namespace test {
Expand Down Expand Up @@ -198,15 +199,14 @@ std::pair<thrust::host_vector<T>, std::vector<bitmask_type>> to_host(column_view
template <>
inline std::pair<thrust::host_vector<std::string>, std::vector<bitmask_type>> to_host(column_view c)
{
auto strings_data = cudf::strings::create_offsets(strings_column_view(c));
thrust::host_vector<char> h_chars(strings_data.first.size());
thrust::host_vector<size_type> h_offsets(strings_data.second.size());
CUDA_TRY(
cudaMemcpy(h_chars.data(), strings_data.first.data(), h_chars.size(), cudaMemcpyDeviceToHost));
CUDA_TRY(cudaMemcpy(h_offsets.data(),
strings_data.second.data(),
h_offsets.size() * sizeof(cudf::size_type),
cudaMemcpyDeviceToHost));
auto const scv = strings_column_view(c);
auto const h_chars = cudf::detail::make_std_vector_sync<char>(
cudf::device_span<char const>(scv.chars().data<char>(), scv.chars().size()),
rmm::cuda_stream_default);
auto const h_offsets = cudf::detail::make_std_vector_sync(
cudf::device_span<cudf::offset_type const>(
scv.offsets().data<cudf::offset_type>() + scv.offset(), scv.size() + 1),
rmm::cuda_stream_default);
Comment on lines +202 to +209
Copy link
Contributor

Choose a reason for hiding this comment

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

Humm, I think we should better use 2 calls to make_vector_async then call stream sync, because 2 calls to make_vector_sync will make 2 implicit stream syncs.

Nevertheless, this is in the test namespace thus I'm not worried much about performance.


// build std::string vector from chars and offsets
std::vector<std::string> host_data;
Expand Down
39 changes: 0 additions & 39 deletions cpp/src/strings/strings_column_view.cu
Original file line number Diff line number Diff line change
Expand Up @@ -54,43 +54,4 @@ size_type strings_column_view::chars_size() const noexcept
return chars().size();
}

namespace strings {

std::pair<rmm::device_uvector<char>, rmm::device_uvector<size_type>> create_offsets(
strings_column_view const& strings,
rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
CUDF_FUNC_RANGE();
size_type const count = strings.size();

auto d_offsets = strings.offsets().data<int32_t>();
d_offsets += strings.offset(); // nvbug-2808421 : do not combine with the previous line

rmm::device_uvector<size_type> offsets(count + 1, stream);
// normalize the offset values for the column offset
thrust::transform(rmm::exec_policy(stream),
d_offsets,
d_offsets + count + 1,
offsets.begin(),
[d_offsets] __device__(int32_t offset) {
return static_cast<size_type>(offset - d_offsets[0]);
});

// get the input chars column byte offset
auto const bytes = offsets.element(count, stream);
auto const chars_offset =
cudf::detail::get_value<offset_type>(strings.offsets(), strings.offset(), stream);
stream.synchronize();

// copy the chars column data
const char* d_chars = strings.chars().data<char>() + chars_offset;
rmm::device_uvector<char> chars(bytes, stream);
CUDA_TRY(cudaMemcpyAsync(chars.data(), d_chars, bytes, cudaMemcpyDefault, stream.value()));

// return offsets and chars
return std::make_pair(std::move(chars), std::move(offsets));
}

} // namespace strings
} // namespace cudf
48 changes: 16 additions & 32 deletions cpp/tests/strings/factories_test.cu
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,14 @@ TEST_F(StringsFactoriesTest, CreateColumnFromPair)
EXPECT_EQ(strings_view.chars().size(), memsize);

// check string data
auto strings_data = cudf::strings::create_offsets(strings_view);
auto h_chars_data = cudf::detail::make_std_vector_sync(strings_data.first);
auto h_offsets_data = cudf::detail::make_std_vector_sync(strings_data.second);
auto h_chars_data = cudf::detail::make_std_vector_sync(
cudf::device_span<char const>(strings_view.chars().data<char>(), strings_view.chars().size()),
rmm::cuda_stream_default);
auto h_offsets_data = cudf::detail::make_std_vector_sync(
cudf::device_span<cudf::offset_type const>(
strings_view.offsets().data<cudf::offset_type>() + strings_view.offset(),
strings_view.size() + 1),
rmm::cuda_stream_default);
EXPECT_EQ(memcmp(h_buffer.data(), h_chars_data.data(), h_buffer.size()), 0);
EXPECT_EQ(
memcmp(h_offsets.data(), h_offsets_data.data(), h_offsets.size() * sizeof(cudf::size_type)), 0);
Expand Down Expand Up @@ -149,9 +154,14 @@ TEST_F(StringsFactoriesTest, CreateColumnFromOffsets)
EXPECT_EQ(strings_view.chars().size(), memsize);

// check string data
auto strings_data = cudf::strings::create_offsets(strings_view);
auto h_chars_data = cudf::detail::make_std_vector_sync(strings_data.first);
auto h_offsets_data = cudf::detail::make_std_vector_sync(strings_data.second);
auto h_chars_data = cudf::detail::make_std_vector_sync(
cudf::device_span<char const>(strings_view.chars().data<char>(), strings_view.chars().size()),
rmm::cuda_stream_default);
auto h_offsets_data = cudf::detail::make_std_vector_sync(
cudf::device_span<cudf::offset_type const>(
strings_view.offsets().data<cudf::offset_type>() + strings_view.offset(),
strings_view.size() + 1),
rmm::cuda_stream_default);
EXPECT_EQ(memcmp(h_buffer.data(), h_chars_data.data(), h_buffer.size()), 0);
EXPECT_EQ(
memcmp(h_offsets.data(), h_offsets_data.data(), h_offsets.size() * sizeof(cudf::size_type)), 0);
Expand Down Expand Up @@ -183,32 +193,6 @@ TEST_F(StringsFactoriesTest, EmptyStringsColumn)
cudf::test::expect_strings_empty(results->view());
}

TEST_F(StringsFactoriesTest, CreateOffsets)
{
std::vector<std::string> strings = {"this", "is", "a", "column", "of", "strings"};
cudf::test::strings_column_wrapper sw = {strings.begin(), strings.end()};
cudf::column_view col(sw);
std::vector<cudf::size_type> indices{0, 2, 3, 6};
auto result = cudf::slice(col, indices);

std::vector<std::vector<std::string>> expecteds{
std::vector<std::string>{"this", "is"}, // [0,2)
std::vector<std::string>{"column", "of", "strings"} // [3,6)
};
for (size_t idx = 0; idx < result.size(); idx++) {
auto strings_data = cudf::strings::create_offsets(cudf::strings_column_view(result[idx]));
auto h_chars = cudf::detail::make_std_vector_sync(strings_data.first);
auto h_offsets = cudf::detail::make_std_vector_sync(strings_data.second);
auto expected_strings = expecteds[idx];
for (size_t jdx = 0; jdx < h_offsets.size() - 1; ++jdx) {
auto offset = h_offsets[jdx];
auto length = h_offsets[jdx + 1] - offset;
std::string str(h_chars.data() + offset, length);
EXPECT_EQ(str, expected_strings[jdx]);
}
}
}

namespace {
using string_pair = thrust::pair<char const*, cudf::size_type>;
struct string_view_to_pair {
Expand Down