Add groupby::replace_nulls(replace_policy) api

[isVoid]

Part 1 of #4896, follow up of #6907

This PR provides a groupby version of the replace_nulls(replace_policy) function. A regular replace_nulls(replace_policy) operation updates the nulls with the first non-null value that precedes/follows the null. The groupby version is similar, with an exception that the non-null value look-up is bounded by groups.

Here is an example to illustrate the API input/output behavior:

#Input:
keys = [2, 1, 2, 1]
values = [3, 4, NULL, NULL]

#Output, group order is not guaranteed:
sorted_keys = [1, 1, 2, 2]
result = [4, 4, 3, 3]

[isVoid]

Requires NVIDIA/thrust#1374

[jrhemstad]

This approach seems sub-optimal to me as it requires deep-copying the group labels just to do a groupby fillna. If instead you could request the fillna directly from the groupby object you could avoid that deep copy.

[isVoid]

This approach seems sub-optimal to me as it requires deep-copying the group labels just to do a groupby fillna. If instead you could request the fillna directly from the groupby object you could avoid that deep copy.

I thought about both - current impl seems less intrusive to the groupby API. I was looking at groupby::get_groups and assumed retrieving groupby meta information is a common practice. But it certainly has performance concerns. Will address on next commit.

[harrism]

@isVoid can you add more clarity to the PR description? In particular, I don't understand what "segmented null replacement" means. Are you replacing nulls in only some groups? If it is all groups, then I would think a regular "flat" null replacement would work.

[isVoid]

@isVoid can you add more clarity to the PR description? In particular, I don't understand what "segmented null replacement" means. Are you replacing nulls in only some groups? If it is all groups, then I would think a regular "flat" null replacement would work.

@harrism Yes, it includes all groups. Does this look good?

[harrism]

Since the documentation is not written yet, I still don't understand, but I'll wait.

[isVoid]

I'm planning to add more tests for lists column and structs column (they should trivially pass because current implementation depends on gather, which supports these column types). Besides where this PR currently stands should be good for review.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.06@0ebf7e6). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-21.06    #7118   +/-   ##
===============================================
  Coverage                ?   82.89%           
===============================================
  Files                   ?      105           
  Lines                   ?    17875           
  Branches                ?        0           
===============================================
  Hits                    ?    14818           
  Misses                  ?     3057           
  Partials                ?        0           

---

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 0ebf7e6...7a90f52. Read the comment docs.

[nvdbaranec]

Couple of very tiny things.

[brandon-b-miller]

Still scratching my head to make sure but I think this might unnecessarily materialize a RangeIndex.

[isVoid]

It might, but it depends on whether the upstream passes in a RangeIndex in values. Here the meaning of values are the value columns in the groupby.replace_nulls operation, RangeIndex shouldn't exist. (Python interface is yet to be added so it is a bit hard to see).

[brandon-b-miller]

One question otherwise cython LGTM

[jrhemstad]

@gpucibot merge