Skip to content

Commit 9631660

Browse files
authored
Fix missing null_count() comparison in test framework and related failures (#7219)
Fixes #7210 Fixes #6733 List of fixes included: - [x] Restore `null_count()` check in `expect_columns_equal` / `expect_columns_equivalent` - [x] Fix issue in `structs_column_view::get_sliced_child` - [x] Fix test failures in COPYING_TEST - [x] Fix test failures in STREAM_COMPACTION_TEST - [x] Fix test failures in RESHAPE_TEST Authors: - @nvdbaranec - Mark Harris (@harrism) Approvers: - Mark Harris (@harrism) - MithunR (@mythrocks) - Jake Hemstad (@jrhemstad) URL: #7219
1 parent dd1efe1 commit 9631660

File tree

6 files changed

+25
-12
lines changed

6 files changed

+25
-12
lines changed

cpp/include/cudf/detail/copy_if.cuh

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ __launch_bounds__(block_size) __global__
9999
{
100100
T* __restrict__ output_data = output_view.data<T>();
101101
cudf::bitmask_type* __restrict__ output_valid = output_view.null_mask();
102-
constexpr cudf::size_type leader_lane{0};
103102
static_assert(block_size <= 1024, "Maximum thread block size exceeded");
104103

105104
int tid = threadIdx.x + per_thread * block_size * blockIdx.x;
@@ -109,8 +108,8 @@ __launch_bounds__(block_size) __global__
109108
__shared__ bool temp_valids[has_validity ? block_size + cudf::detail::warp_size : 1];
110109
__shared__ T temp_data[block_size];
111110

112-
cudf::size_type warp_valid_counts{0};
113-
cudf::size_type block_sum = 0;
111+
cudf::size_type warp_valid_counts{0}; // total valid sum over the `per_thread` loop below
112+
cudf::size_type block_sum = 0; // count passing filter over the `per_thread` loop below
114113

115114
// Note that since the maximum gridDim.x on all supported GPUs is as big as
116115
// cudf::size_type, this loop is sufficient to cover our maximum column size
@@ -160,6 +159,8 @@ __launch_bounds__(block_size) __global__
160159
const int wid = threadIdx.x / cudf::detail::warp_size;
161160
const int lane = threadIdx.x % cudf::detail::warp_size;
162161

162+
cudf::size_type tmp_warp_valid_counts{0};
163+
163164
if (tmp_block_sum > 0 && wid <= last_warp) {
164165
int valid_index = (block_offset / cudf::detail::warp_size) + wid;
165166

@@ -168,9 +169,8 @@ __launch_bounds__(block_size) __global__
168169

169170
// Note the atomicOr's below assume that output_valid has been set to
170171
// all zero before the kernel
171-
172172
if (lane == 0 && valid_warp != 0) {
173-
warp_valid_counts = __popc(valid_warp);
173+
tmp_warp_valid_counts = __popc(valid_warp);
174174
if (wid > 0 && wid < last_warp)
175175
output_valid[valid_index] = valid_warp;
176176
else {
@@ -182,19 +182,22 @@ __launch_bounds__(block_size) __global__
182182
if ((wid == 0) && (last_warp == num_warps)) {
183183
uint32_t valid_warp = __ballot_sync(0xffffffff, temp_valids[block_size + threadIdx.x]);
184184
if (lane == 0 && valid_warp != 0) {
185-
warp_valid_counts += __popc(valid_warp);
185+
tmp_warp_valid_counts += __popc(valid_warp);
186186
atomicOr(&output_valid[valid_index + num_warps], valid_warp);
187187
}
188188
}
189189
}
190+
warp_valid_counts += tmp_warp_valid_counts;
190191
}
191192

192193
block_offset += tmp_block_sum;
193194
tid += block_size;
194195
}
195196
// Compute total null_count for this block and add it to global count
197+
constexpr cudf::size_type leader_lane{0};
196198
cudf::size_type block_valid_count =
197199
cudf::detail::single_lane_block_sum_reduce<block_size, leader_lane>(warp_valid_counts);
200+
198201
if (threadIdx.x == 0) { // one thread computes and adds to null count
199202
atomicAdd(output_null_count, block_sum - block_valid_count);
200203
}

cpp/src/structs/structs_column_view.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,9 @@ column_view structs_column_view::get_sliced_child(int index) const
3636
size(),
3737
child(index).head<uint8_t>(),
3838
child(index).null_mask(),
39-
child(index).null_count(),
39+
// TODO: could potentially compute the actual count here, but at
40+
// the moment this interface doesn't take a stream.
41+
UNKNOWN_NULL_COUNT,
4042
offset(),
4143
children};
4244
}

cpp/tests/copying/scatter_list_tests.cu

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,9 @@ TYPED_TEST(TypedScatterListsTest, ListsOfNullStructs)
733733
};
734734
// clang-format on
735735

736-
auto expected_structs = structs_column_wrapper{{expected_numerics, expected_strings}};
736+
auto expected_structs =
737+
structs_column_wrapper{{expected_numerics, expected_strings},
738+
make_counting_transform_iterator(0, [](auto i) { return i != 6; })};
737739

738740
auto expected_lists = cudf::make_lists_column(
739741
6, offsets_column{0, 3, 5, 9, 11, 13, 15}.release(), expected_structs.release(), 0, {});
@@ -828,7 +830,9 @@ TYPED_TEST(TypedScatterListsTest, EmptyListsOfStructs)
828830
};
829831
// clang-format on
830832

831-
auto expected_structs = structs_column_wrapper{{expected_numerics, expected_strings}};
833+
auto expected_structs =
834+
structs_column_wrapper{{expected_numerics, expected_strings},
835+
make_counting_transform_iterator(0, [](auto i) { return i != 6; })};
832836

833837
auto expected_lists = cudf::make_lists_column(
834838
6, offsets_column{0, 3, 5, 9, 11, 11, 13}.release(), expected_structs.release(), 0, {});
@@ -929,7 +933,9 @@ TYPED_TEST(TypedScatterListsTest, NullListsOfStructs)
929933
};
930934
// clang-format on
931935

932-
auto expected_structs = structs_column_wrapper{{expected_numerics, expected_strings}};
936+
auto expected_structs =
937+
structs_column_wrapper{{expected_numerics, expected_strings},
938+
make_counting_transform_iterator(0, [](auto i) { return i != 6; })};
933939

934940
auto expected_lists_null_mask_begin =
935941
make_counting_transform_iterator(0, [](auto i) { return i != 4; });

cpp/tests/reshape/byte_cast_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ TEST_F(ByteCastTest, int32ValuesWithNulls)
114114
5,
115115
std::move(fixed_width_column_wrapper<cudf::size_type>{0, 4, 8, 12, 16, 20}.release()),
116116
std::move(int32_data.release()),
117-
3,
117+
2,
118118
detail::make_null_mask(even_validity, even_validity + 5));
119119

120120
auto const output_int32 = cudf::byte_cast(int32_col, cudf::flip_endianness::YES);

cpp/tests/stream_compaction/apply_boolean_mask_tests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,7 +249,7 @@ TEST_F(ApplyBooleanMask, NoNullInput)
249249

250250
TEST_F(ApplyBooleanMask, CorrectNullCount)
251251
{
252-
cudf::size_type inputRows = 75000;
252+
cudf::size_type inputRows = 471234;
253253

254254
auto seq1 = cudf::test::make_counting_transform_iterator(0, [](auto i) { return i; });
255255
auto valid_seq1 = cudf::test::make_counting_transform_iterator(0, [](auto row) { return true; });

cpp/tests/utilities/column_utilities.cu

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ struct column_property_comparator {
6767

6868
if (lhs.size() > 0 && check_exact_equality) { EXPECT_EQ(lhs.nullable(), rhs.nullable()); }
6969

70+
EXPECT_EQ(lhs.null_count(), rhs.null_count());
71+
7072
// equivalent, but not exactly equal columns can have a different number of children if their
7173
// sizes are both 0. Specifically, empty string columns may or may not have children.
7274
if (check_exact_equality || lhs.size() > 0) {

0 commit comments

Comments
 (0)