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

Fix null_equality config of rolling_collect_set #8415

Merged
merged 6 commits into from
Jun 3, 2021
Merged

Fix null_equality config of rolling_collect_set #8415

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b7b2bf3
fix rolling_collect_set
sperlingxx Jun 1, 2021
2f380b6
Apply suggestions from code review
sperlingxx Jun 2, 2021
106f2d8
Merge remote-tracking branch 'origin/branch-21.08' into fix_rolling_c…
sperlingxx Jun 3, 2021
19da9de
make up some tests
sperlingxx Jun 3, 2021
a010d8d
Merge branch 'fix_rolling_collect_set' of https://github.com/sperling…
sperlingxx Jun 3, 2021
0e665b0
fix tests
sperlingxx Jun 3, 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
7 changes: 2 additions & 5 deletions cpp/src/rolling/rolling_detail.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,11 +746,8 @@ class rolling_aggregation_postprocessor final : public cudf::detail::aggregation
stream,
rmm::mr::get_current_device_resource());

result = lists::detail::drop_list_duplicates(lists_column_view(collected_list->view()),
null_equality::EQUAL,
nan_equality::UNEQUAL,
stream,
mr);
result = lists::detail::drop_list_duplicates(
lists_column_view(collected_list->view()), agg._nulls_equal, agg._nans_equal, stream, mr);
}

std::unique_ptr<column> get_result()
Expand Down
41 changes: 41 additions & 0 deletions cpp/tests/groupby/collect_set_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,47 @@ TEST_F(CollectSetTest, StringInput)
test_single_agg(keys, vals, keys_expected, vals_expected, CollectSetTest::collect_set());
}

TEST_F(CollectSetTest, FloatsWithNaN)
{
COL_K keys{1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1};
cudf::test::fixed_width_column_wrapper<float> vals{
{1.0f, 1.0f, -2.3e-5f, -2.3e-5f, 2.3e5f, 2.3e5f, -NAN, -NAN, NAN, NAN, 0.0f, 0.0f},
{true, true, true, true, true, true, true, true, true, true, false, false}};
COL_K keys_expected{1};
// null equal with nan unequal
cudf::test::lists_column_wrapper<float> vals_expected{
{{-2.3e-5f, 1.0f, 2.3e5f, -NAN, -NAN, NAN, NAN, 0.0f},
VALIDITY{true, true, true, true, true, true, true, false}},
};
test_single_agg(keys, vals, keys_expected, vals_expected, CollectSetTest::collect_set());
// null unequal with nan unequal
vals_expected = {{{-2.3e-5f, 1.0f, 2.3e5f, -NAN, -NAN, NAN, NAN, 0.0f, 0.0f},
VALIDITY{true, true, true, true, true, true, true, false, false}}};
test_single_agg(
keys, vals, keys_expected, vals_expected, CollectSetTest::collect_set_null_unequal());
// null exclude with nan unequal
vals_expected = {{-2.3e-5f, 1.0f, 2.3e5f, -NAN, -NAN, NAN, NAN}};
test_single_agg(
keys, vals, keys_expected, vals_expected, CollectSetTest::collect_set_null_exclude());
// null equal with nan unequal
sperlingxx marked this conversation as resolved.
Show resolved Hide resolved
vals_expected = {{{-2.3e-5f, 1.0f, 2.3e5f, NAN, 0.0f}, VALIDITY{true, true, true, true, false}}};
test_single_agg(keys,
vals,
keys_expected,
vals_expected,
cudf::make_collect_set_aggregation(
null_policy::INCLUDE, null_equality::EQUAL, nan_equality::ALL_EQUAL));
// null unequal with nan unequal
sperlingxx marked this conversation as resolved.
Show resolved Hide resolved
vals_expected = {
{{-2.3e-5f, 1.0f, 2.3e5f, -NAN, 0.0f, 0.0f}, VALIDITY{true, true, true, true, false, false}}};
test_single_agg(keys,
vals,
keys_expected,
vals_expected,
cudf::make_collect_set_aggregation(
null_policy::INCLUDE, null_equality::UNEQUAL, nan_equality::ALL_EQUAL));
}

TYPED_TEST(CollectSetTypedTest, CollectWithNulls)
{
// Just use an arbitrary value to store null entries
Expand Down
102 changes: 91 additions & 11 deletions cpp/tests/rolling/collect_ops_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1661,16 +1661,16 @@ TYPED_TEST(TypedCollectSetTest, BasicGroupedRollingWindowWithNulls)

using T = TypeParam;

auto const group_column = fixed_width_column_wrapper<int32_t>{1, 1, 1, 1, 1, 2, 2, 2, 2};
auto const group_column = fixed_width_column_wrapper<int32_t>{1, 1, 1, 1, 1, 2, 2, 2, 2, 2};
auto const input_column = fixed_width_column_wrapper<T, int32_t>{
{10, 11, 12, 13, 13, 20, 21, 21, 23}, {1, 0, 0, 1, 1, 1, 0, 1, 1}};
{10, 0, 0, 13, 13, 20, 21, 0, 0, 23}, {1, 0, 0, 1, 1, 1, 1, 0, 0, 1}};

auto const preceding = 2;
auto const following = 1;
auto const min_periods = 1;

{
// Nulls included.
// Nulls included and nulls are equal.
auto const result =
grouped_rolling_window(table_view{std::vector<column_view>{group_column}},
input_column,
Expand All @@ -1679,10 +1679,78 @@ TYPED_TEST(TypedCollectSetTest, BasicGroupedRollingWindowWithNulls)
min_periods,
*make_collect_set_aggregation<rolling_aggregation>());
// Null values are sorted to the tails of lists (sets)
auto expected_child = fixed_width_column_wrapper<T, int32_t>{
{10, 11, 10, 11, 13, 11, 13, 12, 13, 20, 21, 20, 21, 21, 21, 23, 21, 21, 23},
{1, 0, 1, 0, 1, 0, 1, 0, 1, 1, 0, 1, 1, 0, 1, 1, 0, 1, 1}};
auto expected_offsets = fixed_width_column_wrapper<int32_t>{0, 2, 4, 6, 8, 9, 11, 14, 17, 19};
auto expected_child = fixed_width_column_wrapper<T, int32_t>{{
10, 0, // row 0
10, 0, // row 1
13, 0, // row 2
13, 0, // row 3
13, // row 4
20, 21, // row 5
20, 21, 0, // row 6
21, 0, // row 7
23, 0, // row 8
23, 0, // row 9
},
{
1, 0, // row 0
1, 0, // row 1
1, 0, // row 2
1, 0, // row 3
1, // row 4
1, 1, // row 5
1, 1, 0, // row 6
1, 0, // row 7
1, 0, // row 8
1, 0 // row 9
}};
auto expected_offsets =
fixed_width_column_wrapper<int32_t>{0, 2, 4, 6, 8, 9, 11, 14, 16, 18, 20};

auto expected_result = make_lists_column(static_cast<column_view>(group_column).size(),
expected_offsets.release(),
expected_child.release(),
0,
{});

CUDF_TEST_EXPECT_COLUMNS_EQUIVALENT(expected_result->view(), result->view());
}

{
// Nulls included and nulls are NOT equal.
auto const result = grouped_rolling_window(table_view{std::vector<column_view>{group_column}},
input_column,
preceding,
following,
min_periods,
*make_collect_set_aggregation<rolling_aggregation>(
null_policy::INCLUDE, null_equality::UNEQUAL));
// Null values are sorted to the tails of lists (sets)
auto expected_child = fixed_width_column_wrapper<T, int32_t>{{
10, 0, // row 0
10, 0, 0, // row 1
13, 0, 0, // row 2
13, 0, // row 3
13, // row 4
20, 21, // row 5
20, 21, 0, // row 6
21, 0, 0, // row 7
23, 0, 0, // row 8
23, 0 // row 9
},
{
1, 0, // row 0
1, 0, 0, // row 1
1, 0, 0, // row 2
1, 0, // row 3
1, // row 4
1, 1, // row 5
1, 1, 0, // row 6
1, 0, 0, // row 7
1, 0, 0, // row 8
1, 0 // row 9
}};
auto expected_offsets =
fixed_width_column_wrapper<int32_t>{0, 2, 5, 8, 10, 11, 13, 16, 19, 22, 24};

auto expected_result = make_lists_column(static_cast<column_view>(group_column).size(),
expected_offsets.release(),
Expand All @@ -1703,10 +1771,22 @@ TYPED_TEST(TypedCollectSetTest, BasicGroupedRollingWindowWithNulls)
min_periods,
*make_collect_set_aggregation<rolling_aggregation>(null_policy::EXCLUDE));

auto expected_child =
fixed_width_column_wrapper<T, int32_t>{10, 10, 13, 13, 13, 20, 20, 21, 21, 23, 21, 23};

auto expected_offsets = fixed_width_column_wrapper<int32_t>{0, 1, 2, 3, 4, 5, 6, 8, 10, 12};
auto expected_child = fixed_width_column_wrapper<T, int32_t>{
10, // row 0
10, // row 1
13, // row 2
13, // row 3
13, // row 4
20,
21, // row 5
20,
21, // row 6
21, // row 7
23, // row 8
23 // row 9
};

auto expected_offsets = fixed_width_column_wrapper<int32_t>{0, 1, 2, 3, 4, 5, 7, 9, 10, 11, 12};

auto expected_result = make_lists_column(static_cast<column_view>(group_column).size(),
expected_offsets.release(),
Expand Down