Update `bitmask_and` and `bitmask_or` to return a pair of resulting mask and count of unset bits #9616

PointKernel · 2021-11-05T00:50:27Z

Update bitmask_and and bitmask_or to return both resulting mask and count of unset bits
Refactor related implementations to use new bitmask_and/or
Update unit tests

codecov · 2021-11-05T03:33:36Z

Codecov Report

Merging #9616 (8844c4e) into branch-21.12 (ab4bfaa) will decrease coverage by 0.10%.
The diff coverage is n/a.

❗ Current head 8844c4e differs from pull request most recent head 9baa00a. Consider uploading reports for the commit 9baa00a to get more accurate results

@@               Coverage Diff                @@
##           branch-21.12    #9616      +/-   ##
================================================
- Coverage         10.79%   10.68%   -0.11%     
================================================
  Files               116      117       +1     
  Lines             18869    19867     +998     
================================================
+ Hits               2036     2123      +87     
- Misses            16833    17744     +911

Impacted Files	Coverage Δ
python/dask_cudf/dask_cudf/sorting.py	`92.90% <0.00%> (-1.21%)`	⬇️
python/cudf/cudf/io/csv.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/io/hdf.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/io/orc.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/__init__.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/_version.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/abc.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/api/types.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/io/dlpack.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/frame.py	`0.00% <0.00%> (ø)`
... and 67 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f041a47...9baa00a. Read the comment docs.

cpp/include/cudf/detail/null_mask.hpp

hyperbolic2346

Looking good.

cpp/src/groupby/sort/aggregate.cpp

hyperbolic2346

Thanks for the good work. This is a nice cleanup!

bdice · 2021-11-06T00:55:02Z

Quick high level feedback — proper review to follow.

I’m a little unsure about the new struct being named bitmask. It seems like we normally define the RMM device buffer as “the mask” and this name is very generic. Also I’m a little unsure about what the invariants of this type should be. I saw one instance where both the set bits and unset bits were zero, but I expected the invariant (set + unset = total). I feel like this struct might need to carry information about the number of total bits, since that cannot be known from only the device buffer (it’s a byte buffer but the relevant size is in bits and may not align). I’ll think some more and read this more closely on Monday.

edit: My concern about the invariants of the type might have been invalid. I think I overlooked that the set and unset bits were both zero in a case where the code exited early because the total number of rows was also zero. 😛 This looked fine in the other places I saw.

bdice

Nice job @PointKernel. I have some suggestions and questions attached. I thought some more since my previous comment and I'm okay with naming the resulting struct bitmask (I couldn't come up with a better name).

cpp/include/cudf/detail/null_mask.cuh

cpp/src/bitmask/null_mask.cu

cpp/src/groupby/hash/groupby.cu

cpp/include/cudf/null_mask.hpp

hyperbolic2346

I like this much better without struct bitmask. I also lament not being able to name the returns well, but I feel like a pair of two things is much easier to reason about here.

bdice

Great work @PointKernel! I am really happy with how this turned out. I have a couple final comments, resolve them however you see fit. 🚀

cpp/include/cudf/detail/null_mask.cuh

cpp/include/cudf/null_mask.hpp

cpp/src/binaryop/binaryop.cpp

bdice · 2021-11-10T03:58:14Z

cpp/src/structs/utilities.cpp

+                                                              mr);
+      ret_validity_buffers.push_back(std::move(new_mask));
+      return std::make_pair(
+        reinterpret_cast<bitmask_type const*>(ret_validity_buffers.back().data()), null_count);


Consider static_cast here, which should be safer than reinterpret_cast for the void* returned by rmm::device_buffer's data() method.

Suggested change

reinterpret_cast<bitmask_type const*>(ret_validity_buffers.back().data()), null_count);

static_cast<bitmask_type const*>(ret_validity_buffers.back().data()), null_count);

Side note: There are two other reinterpret_casts in this file that I think are safe to delete (their values already seem to be the right types) if we wish to eliminate the appearance of potentially unsafe reinterpret_casts in this file.

cpp/include/cudf/detail/null_mask.cuh

cpp/include/cudf/null_mask.hpp

PointKernel · 2021-11-11T18:49:33Z

@gpucibot merge

This PR updates the function `cudf::detail::inplace_bitmask_and` to return the null count of the result. This change aligns `inplace_bitmask_and` with behavior changes introduced in #9616 to return null counts from functions acting on bitmasks. This will be helpful for #9621. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Nghia Truong (https://github.com/ttnghia) - Robert Maynard (https://github.com/robertmaynard) URL: #9904

PointKernel added 2 commits November 4, 2021 19:45

Updates: bitmask_and returns output mask and count of valid bits

c4e8dc0

Update group sort to use new bitmask_and

0051831

PointKernel added feature request New feature or request 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function breaking Breaking change labels Nov 5, 2021

PointKernel requested a review from a team November 5, 2021 00:50

PointKernel self-assigned this Nov 5, 2021

PointKernel requested review from hyperbolic2346 and bdice and removed request for a team November 5, 2021 00:50

PointKernel removed the improvement Improvement / enhancement to an existing function label Nov 5, 2021

jrhemstad reviewed Nov 5, 2021

View reviewed changes

cpp/include/cudf/detail/null_mask.hpp Show resolved Hide resolved

hyperbolic2346 requested changes Nov 5, 2021

View reviewed changes

cpp/src/groupby/sort/aggregate.cpp Outdated Show resolved Hide resolved

PointKernel added 2 commits November 5, 2021 13:43

Updates: bitmask_and return a struct of mask, valid count and null count

9442b0d

Refactor related functions with new bitmask_and

7d23f99

PointKernel requested a review from a team as a code owner November 5, 2021 19:06

Update unit tests

5abaf26

PointKernel changed the title ~~[WIP] Update bitmask_and to return both resulting mask and count of valid bits~~ Update bitmask_and to return a struct of resulting mask, count of valid bits, and count of null bits Nov 5, 2021

PointKernel added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Nov 5, 2021

hyperbolic2346 approved these changes Nov 6, 2021

View reviewed changes

Fix a repeated string bug

4d60f87

Fix a slack bit bug when counting valid bits

8981f5d

bdice requested changes Nov 9, 2021

View reviewed changes

github-actions bot added the Java Affects Java cuDF API. label Nov 9, 2021

Code formatting

362eb85

PointKernel changed the title ~~Update bitmask_and to return a struct of resulting mask, count of valid bits, and count of null bits~~ Update bitmask_and and bitmask_or to return a struct of resulting mask, valid count, and null count Nov 9, 2021

jrhemstad reviewed Nov 9, 2021

View reviewed changes

cpp/include/cudf/null_mask.hpp Outdated Show resolved Hide resolved

PointKernel changed the title ~~Update bitmask_and and bitmask_or to return a struct of resulting mask, valid count, and null count~~ Update bitmask_and and bitmask_or to return a struct of resulting mask and null count Nov 10, 2021

PointKernel changed the title ~~Update bitmask_and and bitmask_or to return a struct of resulting mask and null count~~ Update bitmask_and and bitmask_or to return a pair of resulting mask and null count Nov 10, 2021

Updates: return a pair of mask and null count

bf1bca0

hyperbolic2346 approved these changes Nov 10, 2021

View reviewed changes

bdice approved these changes Nov 10, 2021

View reviewed changes

jrhemstad reviewed Nov 10, 2021

View reviewed changes

cpp/include/cudf/detail/null_mask.cuh Outdated Show resolved Hide resolved

jrhemstad reviewed Nov 10, 2021

View reviewed changes

cpp/include/cudf/detail/null_mask.cuh Outdated Show resolved Hide resolved

cpp/include/cudf/null_mask.hpp Outdated Show resolved Hide resolved

PointKernel added the 4 - Needs cuDF (Java) Reviewer label Nov 10, 2021

Update docs

9a0f562

PointKernel changed the title ~~Update bitmask_and and bitmask_or to return a pair of resulting mask and null count~~ Update bitmask_and and bitmask_or to return a pair of resulting mask and count of unset bits Nov 10, 2021

Corrections

9baa00a

PointKernel requested review from revans2 and jlowe November 11, 2021 16:58

jlowe approved these changes Nov 11, 2021

View reviewed changes

rapids-bot bot merged commit ba2b51d into rapidsai:branch-21.12 Nov 11, 2021

PointKernel deleted the count-valid-bits branch November 11, 2021 18:49

vyasr mentioned this pull request Nov 11, 2021

Remove sizeof and standardize on memory_usage #9544

Merged

bdice mentioned this pull request Nov 12, 2021

Interchange dataframe protocol #9071

Merged

bdice mentioned this pull request Dec 14, 2021

Return count of set bits from inplace_bitmask_and. #9904

Merged

bdice mentioned this pull request Jul 6, 2022

Struct binary comparison op functionality for spark rapids #11153

Merged

vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuDF (Java) Reviewer labels Feb 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `bitmask_and` and `bitmask_or` to return a pair of resulting mask and count of unset bits #9616

Update `bitmask_and` and `bitmask_or` to return a pair of resulting mask and count of unset bits #9616

PointKernel commented Nov 5, 2021 •

edited

Loading

codecov bot commented Nov 5, 2021 •

edited

Loading

hyperbolic2346 left a comment

hyperbolic2346 left a comment

bdice commented Nov 6, 2021 •

edited

Loading

bdice left a comment

hyperbolic2346 left a comment

bdice left a comment

bdice Nov 10, 2021

PointKernel commented Nov 11, 2021

	reinterpret_cast<bitmask_type const*>(ret_validity_buffers.back().data()), null_count);
	static_cast<bitmask_type const*>(ret_validity_buffers.back().data()), null_count);

Update bitmask_and and bitmask_or to return a pair of resulting mask and count of unset bits #9616

Update bitmask_and and bitmask_or to return a pair of resulting mask and count of unset bits #9616

Conversation

PointKernel commented Nov 5, 2021 • edited Loading

codecov bot commented Nov 5, 2021 • edited Loading

Codecov Report

hyperbolic2346 left a comment

Choose a reason for hiding this comment

hyperbolic2346 left a comment

Choose a reason for hiding this comment

bdice commented Nov 6, 2021 • edited Loading

bdice left a comment

Choose a reason for hiding this comment

hyperbolic2346 left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

bdice Nov 10, 2021

Choose a reason for hiding this comment

PointKernel commented Nov 11, 2021

Update `bitmask_and` and `bitmask_or` to return a pair of resulting mask and count of unset bits #9616

Update `bitmask_and` and `bitmask_or` to return a pair of resulting mask and count of unset bits #9616

PointKernel commented Nov 5, 2021 •

edited

Loading

codecov bot commented Nov 5, 2021 •

edited

Loading

bdice commented Nov 6, 2021 •

edited

Loading