Add Segmented sort #7122

karthikeyann · 2021-01-12T11:08:09Z

addresses part of #6541 Segment sort of lists

lists_column_view segmented_sort
numerical types (cub segmented sort limitation)
sort_lists(table_view)
unit tests

closes #4603 Segmented sort

segmented_sort
unit tests.

…nted_sort_list

cpp/include/cudf/lists/sorting.hpp

cpp/src/lists/segmented_sort.cu

jrhemstad · 2021-01-26T16:35:55Z

cpp/src/sort/segmented_sort.cu

+  CUDF_EXPECTS(std::all_of(keys.begin(),
+                           keys.end(),
+                           [](column_view const& col) { return col.type().id() == type_id::LIST; }),
+               "segmented_sort only supports lists columns");


Why? I'd think I'd should be able to do a segmented sort of a normal column. In fact, I filed an issue asking for exactly that ages ago: #4603

#6541 requested for list columns. This will be renamed to sort_lists and this check will remain.
segmented_sort of normal column/table will be exposed as per #4603 specifications.

implemented segmented_sort as well.

…nted_sort_list

codecov · 2021-01-27T23:02:23Z

Codecov Report

Merging #7122 (2761626) into branch-0.18 (8860baf) will increase coverage by 0.11%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           branch-0.18    #7122      +/-   ##
===============================================
+ Coverage        82.09%   82.20%   +0.11%     
===============================================
  Files               97      100       +3     
  Lines            16474    16952     +478     
===============================================
+ Hits             13524    13936     +412     
- Misses            2950     3016      +66

Impacted Files	Coverage Δ
python/cudf/cudf/__init__.py	`100.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/avro.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/csv.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/fuzzer.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/io.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/json.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/main.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/orc.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/parquet.py	`0.00% <ø> (ø)`
python/cudf/cudf/_fuzz_testing/utils.py	`0.00% <ø> (ø)`
... and 83 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 3ecde9d...e0696ef. Read the comment docs.

cpp/include/cudf/sorting.hpp

jrhemstad · 2021-02-01T22:19:21Z

cpp/include/cudf/sorting.hpp

+  rmm::mr::device_memory_resource* mr            = rmm::mr::get_current_device_resource());
+
+/**
+ * @brief Performs a lexicographic segmented sort of the list in each row of a table of list columns


I don't understand this function. How do you do a lexicographic sort of a table of list columns?

Is this clear?
Performs a lexicographic sort of each list in a table.

So is it the same as calling sort independently on each column?

no. It will be similar to calling sort on each row of the table. Each row is a list.
The relative row order in the table remains same. In each row, the elements in list are sorted.

So it's the same as calling sort_lists on each column in the table?

no. it's similar to treating each row as table, then sort it. then repeat it for all rows.

Copying the example from #6541

row# a b c

0 [21, 22, 23, 22] [13, 14, 12, 11] ["a", "b", "c", "d"]

1 [22, 21, 23, 22] [14, 13, 12, 11] ["a", "b", "c", "d"]

Here [...] is a list.

row# a b c

0
21
22
23
22

13
14
12
11

"a"
"b"
"c"
"d"

1
22
21
23
22

14
13
12
11

"a"
"b"
"c"
"d"

sort_lists(values={a,b,c}, keys={a,b}) output will be

row# a b c

0
21
22
22
23

13
11
14
12

"a"
"d"
"b"
"c"

1
21
22
22
23

13
11
14
12

"b"
"d"
"a"
"c"

Does that require all of the lists in a given row to have the same number of elements? What would the result be for this input?

row# a b c

0 [21, 22, 23, 22] [13, 14] ["a", "c", "d"]

1 [22, 21, 23, 22] [14] ["c", "d"]

Yes.
For this input, it will throw a logic error.

cpp/src/sort/segmented_sort.cu

…nted_sort_list

karthikeyann · 2021-02-02T15:29:37Z

Unrelated style check failures.
FAILED: mypy style check
python/cudf/cudf/core/column/categorical.py:818: error: Item "dtype[Any]" of "Union[Any, dtype[Any]]" has no attribute "serialize"

kkraus14 · 2021-02-02T15:31:11Z

Unrelated style check failures.
FAILED: mypy style check
python/cudf/cudf/core/column/categorical.py:818: error: Item "dtype[Any]" of "Union[Any, dtype[Any]]" has no attribute "serialize"

Being fixed in #7279

jrhemstad

High level questions:

Why is sort_lists different from segmented_sort? Couldn't sort_lists just be implemented by calling segmented_sort on the data/offsets children of a list column?
This brings up the question, do we even need a sort_lists API? Couldn't a caller trivially just do segmented_sort(list.data, list.offsets)? This had the advantage of implicitly enforcing the "lists of depth 1" requirement.
The sort_lists table API still sticks out as very odd to me. I think from [FEA] Segmented sort / sorted_order (argsort) #6541 what @kkraus14 really wanted is just a table level segmented sort, not a lexicographic sort of lists of all the same length.

In summary, all I would think we need is:

segmented_sort_by_key(table_view values, table_view keys, column_view segment_offsets, ...)
segmented_sorted_order(table_view keys, column_view segment_offsets)

karthikeyann · 2021-02-03T03:37:36Z

Had discussion with @jrhemstad

sort_lists has 2 functions/implementations.
- sort_lists(column_view) for single column (faster method, uses cub segmented radix sort, fixed_width columns)
- sort_lists(table_view) (this is implemented by calling segmented_sort_by_key(table_view) on the data/offsets children of a list column)
Caller can call segmented_sort_by_key, but caller needs to add offset to list.offsets, have to reconstruct the list columns of table after getting result, which is what sort_lists(table_view) does.
discussed with @jrhemstad

functions for segmented sorts and sorted_order (argsort) to sort the list elements within a column

sort_lists(table_view) is not needed and will be removed. sort_lists(column_view) is required and will be kept.

harrism · 2021-02-03T04:24:16Z

@karthikeyann can you get the requested changes done by tomorrow? If not we should push to 0.19.

karthikeyann · 2021-02-03T04:28:39Z

@karthikeyann can you get the requested changes done by tomorrow? If not we should push to 0.19.

Yes. It will be done by today. By tomorrow, I will address re-review comments as well.

codereport

👍

kkraus14 · 2021-02-04T00:50:33Z

rerun tests

kkraus14 · 2021-02-04T00:50:44Z

@gpucibot merge

karthikeyann added 2 commits January 12, 2021 16:33

add segmented sort for lists_column_view

b2464b1

add unit test for segmented sort for lists_column_view

e7e2e4f

karthikeyann added feature request New feature or request 2 - In Progress Currently a work in progress non-breaking Non-breaking change labels Jan 12, 2021

karthikeyann self-assigned this Jan 12, 2021

karthikeyann added 5 commits January 25, 2021 14:04

add segmented_sort(table_view)

f16f924

add unit test segmented_sort(table_view) all valid

44ff02a

fix interfaces

4d5fbbf

add null list column test

edfa549

Merge branch 'branch-0.18' of github.com:rapidsai/cudf into fea-segme…

9a8c9d5

…nted_sort_list

jrhemstad reviewed Jan 26, 2021

View reviewed changes

cpp/include/cudf/lists/sorting.hpp Outdated Show resolved Hide resolved

jrhemstad reviewed Jan 26, 2021

View reviewed changes

cpp/include/cudf/lists/sorting.hpp Outdated Show resolved Hide resolved

jrhemstad requested changes Jan 26, 2021

View reviewed changes

karthikeyann added 5 commits January 27, 2021 23:57

fix null_order example, code, tests for sort_lists segmented sort

54c0c5e

documentation update for segmented_sort_by_key

4b6ae05

replace segmented_sort by segmented_sort_by_key

a936f4e

Merge branch 'branch-0.18' of github.com:rapidsai/cudf into fea-segme…

2baf424

…nted_sort_list

conda yml include new hpp

d03f032

karthikeyann added 5 commits January 28, 2021 18:38

add more checks for input list table

23c8f26

add error tests for inputs

0825984

rename API segmented_sort to sort_lists

663662b

rename tests

e842e1d

update sort_lists to use segmented_sorted_order

bb0e7c1

karthikeyann marked this pull request as ready for review January 28, 2021 20:29

karthikeyann requested review from a team as code owners January 28, 2021 20:29

jrhemstad reviewed Feb 1, 2021

View reviewed changes

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

jrhemstad requested changes Feb 1, 2021

View reviewed changes

Apply suggestions from code review (jake)

f6c058a

karthikeyann requested a review from jrhemstad February 1, 2021 23:06

codereport suggested changes Feb 1, 2021

View reviewed changes

cpp/src/sort/segmented_sort.cu Outdated Show resolved Hide resolved

cpp/src/sort/segmented_sort.cu Outdated Show resolved Hide resolved

cpp/src/sort/segmented_sort.cu Outdated Show resolved Hide resolved

karthikeyann added 2 commits February 2, 2021 20:45

address review comments (codereport)

df49138

Merge branch 'branch-0.18' of github.com:rapidsai/cudf into fea-segme…

95dda47

…nted_sort_list

karthikeyann changed the title ~~Add Segmented sort for lists~~ Add Segmented sort Feb 2, 2021

karthikeyann requested a review from codereport February 2, 2021 15:30

jrhemstad requested changes Feb 2, 2021

View reviewed changes

karthikeyann added 3 commits February 3, 2021 14:59

remove sort_lists(table_view)

1141485

add cudf::segmented_sorted_order in header

476420d

update copyright year

8732fbb

jrhemstad approved these changes Feb 3, 2021

View reviewed changes

codereport approved these changes Feb 3, 2021

View reviewed changes

kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Feb 3, 2021

add more tests, enable sliced list column

e0696ef

rapids-bot bot merged commit 369ec98 into rapidsai:branch-0.18 Feb 4, 2021

shwina mentioned this pull request Feb 26, 2021

[FEA] Python bindings for lists::sort #7467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Segmented sort #7122

Add Segmented sort #7122

karthikeyann commented Jan 12, 2021 •

edited

Loading

jrhemstad Jan 26, 2021

karthikeyann Jan 26, 2021

karthikeyann Feb 1, 2021

codecov bot commented Jan 27, 2021 •

edited

Loading

jrhemstad Feb 1, 2021

karthikeyann Feb 1, 2021 •

edited

Loading

jrhemstad Feb 1, 2021

karthikeyann Feb 1, 2021

jrhemstad Feb 1, 2021

karthikeyann Feb 2, 2021

jrhemstad Feb 2, 2021

karthikeyann Feb 2, 2021

karthikeyann commented Feb 2, 2021

kkraus14 commented Feb 2, 2021

jrhemstad left a comment

karthikeyann commented Feb 3, 2021

harrism commented Feb 3, 2021

karthikeyann commented Feb 3, 2021

codereport left a comment

kkraus14 commented Feb 4, 2021

kkraus14 commented Feb 4, 2021

row#	a	b	c
0	[21, 22, 23, 22]	[13, 14, 12, 11]	["a", "b", "c", "d"]
1	[22, 21, 23, 22]	[14, 13, 12, 11]	["a", "b", "c", "d"]

row#	a	b	c
0	21 22 23 22	13 14 12 11	"a" "b" "c" "d"
1	22 21 23 22	14 13 12 11	"a" "b" "c" "d"

row#	a	b	c
0	21 22 22 23	13 11 14 12	"a" "d" "b" "c"
1	21 22 22 23	13 11 14 12	"b" "d" "a" "c"

row#	a	b	c
0	[21, 22, 23, 22]	[13, 14]	["a", "c", "d"]
1	[22, 21, 23, 22]	[14]	["c", "d"]

Add Segmented sort #7122

Add Segmented sort #7122

Conversation

karthikeyann commented Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 27, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

karthikeyann Feb 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikeyann commented Feb 2, 2021

kkraus14 commented Feb 2, 2021

jrhemstad left a comment

Choose a reason for hiding this comment

karthikeyann commented Feb 3, 2021

harrism commented Feb 3, 2021

karthikeyann commented Feb 3, 2021

codereport left a comment

Choose a reason for hiding this comment

kkraus14 commented Feb 4, 2021

kkraus14 commented Feb 4, 2021

karthikeyann commented Jan 12, 2021 •

edited

Loading

codecov bot commented Jan 27, 2021 •

edited

Loading

karthikeyann Feb 1, 2021 •

edited

Loading