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

Fix issue in slice() where columns with a positive offset were computing null counts incorrectly. #8738

Merged
merged 4 commits into from
Jul 19, 2021
Merged

Fix issue in slice() where columns with a positive offset were computing null counts incorrectly. #8738

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e70723d
Fix issue in slice() where columns with a positive offset were comput…
nvdbaranec Jul 13, 2021
d8b542b
Formatting fix.
nvdbaranec Jul 13, 2021
56d83e2
Refactor detail implementations of segmented_count_unset_bits() and s…
nvdbaranec Jul 14, 2021
a109b03
Fix bad function name.
nvdbaranec Jul 14, 2021
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
17 changes: 16 additions & 1 deletion cpp/src/copying/slice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,22 @@ std::vector<column_view> slice(column_view const& input,

if (indices.empty()) return {};

auto null_counts = cudf::detail::segmented_count_unset_bits(input.null_mask(), indices, stream);
auto null_counts = [&]() {
// need to shift incoming indices by the column offset to generate the correct bit ranges
// to count
if (input.offset() > 0) {
std::vector<size_type> shifted_indices;
shifted_indices.reserve(indices.size());
std::transform(indices.begin(),
indices.end(),
std::back_inserter(shifted_indices),
[offset = input.offset()](size_type index) { return index + offset; });
Copy link
Contributor

Choose a reason for hiding this comment

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

slice() is supposed to be a relatively free operation (ignoring the bit counting), so materializing the shifted indices is a bummer.

I think it would be fairly easy to make segmented_count_unset_bits take an iterator for the segment offsets so you could instead pass a transform iterator in that adds the offset.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about just invalidating the null count so the slice function will never do such counting? We don't know if the null count will be used later or not. If it is used later, it will be recomputed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We used to do it that way, but we found that it was a better idea to compute all the null counts at once. Otherwise if you have many slices where you need the null count, you end up launching many kernels. This came up in the context of contiguous_split.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: slice.cpp will have to become slice.cu to do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Couple things of note: A number of functions/kernels got moved from null_mask.cu to null_mask.cuh. Both segmented_count_unset_bits and segmented_count_set_bits have been refactored to take iterators, since unset is implemented through set. I had to change a device lambda in segmented_count_unset_bits into a functor (word_num_set_bits_functor) due to extended device lambda restrictions causing the compiler to complain about the iterators now being passed in.

return cudf::detail::segmented_count_unset_bits(input.null_mask(), shifted_indices, stream);
}
// can use the initial indices
return cudf::detail::segmented_count_unset_bits(input.null_mask(), indices, stream);
}();

nvdbaranec marked this conversation as resolved.
Show resolved Hide resolved
auto const children = std::vector<column_view>(input.child_begin(), input.child_end());

auto op = [&](auto i) {
Expand Down
44 changes: 44 additions & 0 deletions cpp/tests/copying/slice_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,3 +493,47 @@ TEST_F(SliceTableCornerCases, MiscOffset)

CUDF_TEST_EXPECT_COLUMNS_EQUAL(col3, result_column);
}

TEST_F(SliceTableCornerCases, PreSlicedInputs)
{
{
using LCW = cudf::test::lists_column_wrapper<float>;

cudf::test::fixed_width_column_wrapper<int> a{{0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
{1, 1, 0, 1, 1, 1, 0, 0, 1, 0}};

cudf::test::fixed_width_column_wrapper<int> b{{0, -1, -2, -3, -4, -5, -6, -7, -8, -9},
{0, 0, 0, 0, 0, 0, 0, 0, 0, 0}};

cudf::test::strings_column_wrapper c{{"aa", "b", "", "ccc", "ddd", "e", "ff", "", "", "gggg"},
{0, 0, 1, 1, 0, 0, 1, 1, 1, 0}};

std::vector<bool> list_validity{1, 0, 1, 0, 1, 1, 0, 0, 1, 1};
cudf::test::lists_column_wrapper<float> d{
{{0, 1}, {2}, {3, 4, 5}, {6}, {7, 7}, {8, 9}, {10, 11}, {12, 13}, {}, {14, 15, 16}},
list_validity.begin()};

cudf::table_view t({a, b, c, d});

auto pre_sliced = cudf::slice(t, {0, 4, 4, 10});

auto result = cudf::slice(pre_sliced[1], {0, 1, 1, 6});

cudf::test::fixed_width_column_wrapper<int> e0_a({4}, {1});
cudf::test::fixed_width_column_wrapper<int> e0_b({-4}, {0});
cudf::test::strings_column_wrapper e0_c({""}, {0});
std::vector<bool> e0_list_validity{1};
cudf::test::lists_column_wrapper<float> e0_d({LCW{7, 7}}, e0_list_validity.begin());
cudf::table_view expected0({e0_a, e0_b, e0_c, e0_d});
CUDF_TEST_EXPECT_TABLES_EQUAL(result[0], expected0);

cudf::test::fixed_width_column_wrapper<int> e1_a{{5, 6, 7, 8, 9}, {1, 0, 0, 1, 0}};
cudf::test::fixed_width_column_wrapper<int> e1_b{{-5, -6, -7, -8, -9}, {0, 0, 0, 0, 0}};
cudf::test::strings_column_wrapper e1_c{{"e", "ff", "", "", "gggg"}, {0, 1, 1, 1, 0}};
std::vector<bool> e1_list_validity{1, 0, 0, 1, 1};
cudf::test::lists_column_wrapper<float> e1_d{{{8, 9}, {10, 11}, {12, 13}, {}, {14, 15, 16}},
e1_list_validity.begin()};
cudf::table_view expected1({e1_a, e1_b, e1_c, e1_d});
CUDF_TEST_EXPECT_TABLES_EQUAL(result[1], expected1);
}
}