Add handling for nulls in dask_cudf.sorting.quantile_divisions

[charlesbluca]

Closes #9157

Originally, dask_cudf.DataFrame.sort_values would fail if the DataFrame had enough null values that divisions contained nulls here:

cudf/python/dask_cudf/dask_cudf/sorting.py

Lines 189 to 191 in 1935a8a

	divisions = sorted(
	divisions.drop_duplicates().astype(dtype).values.tolist()
	)

As you cannot get the values of a Series containing nulls; this PR drops the nulls from divisions before calling values, appending the correct amount to the resulting list afterwards to avoid this failure.

[galipremsagar]

Can we use .to_arrow and sort, that way we are tampering the null ordering?

[charlesbluca]

Were you thinking something like

        divisions = (
            sorted(
                divisions.drop_duplicates()
                .astype(dtype)
                .to_arrow().tolist()
            )

In that case, the null ordering is still being tampered by drop_duplicates, which places nulls first. From there we can't sort the resulting list as it contains None, unless you meant using an arrow sorting method before calling tolist()?

[galipremsagar]

I see, didn't notice the drop_duplicates call, yea drop_duplicates results in non-deterministic ordering. But .to_arrow().tolist() seems cleaner to me.

[charlesbluca]

Okay, in that case do you have any preference for how we handle sorting the list with nulls? Doing a quick search, this seems like a suitable solution:

# https://stackoverflow.com/questions/18411560/sort-list-while-pushing-none-values-to-the-end

sorted(..., key=lambda x: (x is None, x))

[galipremsagar]

Try sort_indices of pyarrow, it seems to be doing what you want to do: https://arrow.apache.org/docs/python/generated/pyarrow.compute.sort_indices.html

If that's not possible the StackOverflow approach looks fine to me.

[codecov]

Codecov Report

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

❗ Current head 96d085f differs from pull request most recent head 3956939. Consider uploading reports for the commit 3956939 to get more accurate results

@@               Coverage Diff               @@
##             branch-21.10    #9171   +/-   ##
===============================================
  Coverage                ?   10.78%           
===============================================
  Files                   ?      115           
  Lines                   ?    19113           
  Branches                ?        0           
===============================================
  Hits                    ?     2062           
  Misses                  ?    17051           
  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 858944b...3956939. Read the comment docs.

[galipremsagar]

rerun tests

[rjzamora]

Seems like a reasonable change to me. Thanks @charlesbluca !

[quasiben]

Thanks @charlesbluca !

[quasiben]

@gpucibot merge