Support `get_element` from LIST column #8071

isVoid · 2021-04-26T23:43:25Z

Part1 of #8032

This PR adds retrieval of row data from a LIST type column, through adding support to list_view specialization of get_element. The row data is stored in a scalar object.

Use example:

// non-nested LIST column
col = [{1, 2, 3}, {4}]
s = get_element(col, 1); // s is a type erased list_scalar, s._data == int_column{4}

// nested LIST column
col = [[{1, 2}, {3}], [{4}, {}]]
s = get_element(col, 1); // s is a type erased list_scalar, s._data == list_column{{4}, {}}

Implementation note:
Depends on lists::detail::copy_slice under the hood. Also adds a new list_scalar constructor that supports moving external row data to construct a new scalar.

Other included in this PR:

is_element_valid_sync(column, i), helper function that returns true if ith row of column is valid.
list_scalar factory functions
Developer guide for list_scalar

codecov · 2021-04-27T03:44:34Z

Codecov Report

Merging #8071 (aa38f7f) into branch-0.20 (51336df) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           branch-0.20    #8071      +/-   ##
===============================================
- Coverage        82.88%   82.88%   -0.01%     
===============================================
  Files              103      104       +1     
  Lines            17668    17899     +231     
===============================================
+ Hits             14645    14836     +191     
- Misses            3023     3063      +40

Impacted Files	Coverage Δ
python/cudf/cudf/core/tools/datetimes.py	`80.42% <0.00%> (-4.11%)`	⬇️
python/cudf/cudf/core/column/decimal.py	`90.64% <0.00%> (-2.29%)`	⬇️
python/cudf/cudf/core/column/datetime.py	`88.03% <0.00%> (-1.88%)`	⬇️
python/cudf/cudf/core/column/struct.py	`94.73% <0.00%> (-1.56%)`	⬇️
python/cudf/cudf/utils/dtypes.py	`82.20% <0.00%> (-1.24%)`	⬇️
python/dask_cudf/dask_cudf/groupby.py	`91.28% <0.00%> (-0.88%)`	⬇️
python/cudf/cudf/core/series.py	`91.17% <0.00%> (-0.56%)`	⬇️
python/cudf/cudf/core/index.py	`92.52% <0.00%> (-0.55%)`	⬇️
python/cudf/cudf/core/column/column.py	`88.20% <0.00%> (-0.44%)`	⬇️
python/cudf/cudf/core/column/lists.py	`86.98% <0.00%> (-0.43%)`	⬇️
... and 28 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 4853dbc...aa38f7f. Read the comment docs.

harrism

Looks fine, just suggest refactoring the single-element is_valid to a utility.

cpp/src/copying/get_element.cu

ttnghia · 2021-04-27T14:41:10Z

The behavior seems not right to me. You have a lists column. You access a row in that list column. That row is always a list. Thus, the return list scalar should always have the _data member is a list column of one row.

isVoid · 2021-04-27T17:22:29Z

@ttnghia currently list_scalar is defined with the top-level list wrapper removed.

For example, in a non-nested list column:

// Lists<int>
row0: [1, 2, 3]
row1: [4]
row2: NULL

get_element(0) should return a `list_scalar` with underlying data of INT column {1, 2, 3}

For a nested list column:

// List<List<int>>
row0:
    row[0][0]: [1, 2, 3]
    row[0][1]: [4, 5]
row1:
    row[1][0]: []
    row[1][1]: NULL

get_element(0) should return `list_scalar` with underlying data of LIST column, which has offset {0, 3, 5} and underlying data of INT column {1, 2, 3, 4, 5}

See discussion here: #5887 (comment)
I guess this saves the overhead of storing an offset vector for the 1-row LIST column, which is always [0, size_of_child] and removes the need of recursion.

isVoid · 2021-04-27T17:31:26Z

I should probably add the definition of list_scalar to the developer guide:
https://github.com/rapidsai/cudf/blob/branch-0.20/cpp/docs/DEVELOPER_GUIDE.md#cudfscalar

cpp/src/scalar/scalar_factories.cpp

cpp/src/bitmask/get_valid.cpp

cpp/include/cudf/detail/get_valid.hpp

…ist_get_element

Co-authored-by: Mark Harris <[email protected]>

nvdbaranec

My comment here still stands. #8071 (comment) We should be using gather.

isVoid · 2021-05-04T17:14:40Z

Added scalar factory methods, and updated developer guide accordingly.

cc @nvdbaranec @harrism

isVoid · 2021-05-04T20:34:19Z

My comment here still stands. #8071 (comment) We should be using gather.

Discussed offline and confirmed current implementation supports structs.

…ist_get_element

ttnghia · 2021-05-06T01:09:25Z

It seems that there are some failed tests.

cpp/include/cudf/scalar/scalar_factories.hpp

Co-authored-by: nvdbaranec <[email protected]>

kkraus14 · 2021-05-07T20:32:33Z

@gpucibot merge

…8206) This PR fixes a bug introduced in #8071, when `get_element` retrieves a NULL row in a nested column, the scalar returned not only should be `is_valid() == false`, but also should preserve the column hierarchy of the row-data, even they are invalid. Because depending libraries may use the column hierarchy to deduce the nested type of the column. This PR also reverts `make_default_constructed_scalar` API for `LIST` type. A `LIST` type scalar should have complete column hierarchy as part of its type information. There isn't enough information provided to the API to construct that. Another tiny addition: instead of hard coding the position of child column, use `list_column_view::child_column_index` intead. Authors: - Michael Wang (https://github.com/isVoid) Approvers: - Conor Hoekstra (https://github.com/codereport) - Paul Taylor (https://github.com/trxcllnt) URL: #8206

Initial

60725de

isVoid requested a review from a team as a code owner April 26, 2021 23:43

isVoid requested review from robertmaynard and harrism April 26, 2021 23:43

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Apr 26, 2021

isVoid added 3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change labels Apr 26, 2021

Remove clangd imports

67c4d04

harrism requested changes Apr 27, 2021

View reviewed changes

cpp/src/copying/get_element.cu Outdated Show resolved Hide resolved

robertmaynard approved these changes Apr 27, 2021

View reviewed changes

isVoid commented Apr 27, 2021

View reviewed changes

cpp/src/scalar/scalar_factories.cpp Outdated Show resolved Hide resolved

isVoid added 2 - In Progress Currently a work in progress and removed 3 - Ready for Review Ready for review by team labels Apr 28, 2021

robertmaynard approved these changes Apr 28, 2021

View reviewed changes

isVoid added 2 commits April 28, 2021 12:42

Copyright years and fix test nuances

daf5c5b

Add get_valid API

8d2d7ef

isVoid requested a review from a team as a code owner April 28, 2021 23:37

github-actions bot added the CMake CMake build issue label Apr 28, 2021

fixing default scalar factory method for list_scalar

2db0e8d

harrism requested changes Apr 28, 2021

View reviewed changes

cpp/src/bitmask/get_valid.cpp Outdated Show resolved Hide resolved

cpp/include/cudf/detail/get_valid.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/detail/get_valid.hpp Outdated Show resolved Hide resolved

isVoid and others added 3 commits April 28, 2021 16:49

Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into l…

ccbb568

…ist_get_element

doc, func naming review apply

72718e6

Co-authored-by: Mark Harris <[email protected]>

Refactor get_valid into is_element_valid_sync

5a5f1d6

Doc updates

dbb4a69

Co-authored-by: Mark Harris <[email protected]>

isVoid requested a review from nvdbaranec May 4, 2021 01:21

harrism approved these changes May 4, 2021

View reviewed changes

nvdbaranec requested changes May 4, 2021

View reviewed changes

Factories, dev guide updates

982a7c2

Reverting construct from row factory

3a2d0c3

nvdbaranec self-requested a review May 5, 2021 14:38

nvdbaranec approved these changes May 5, 2021

View reviewed changes

Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into l…

5cffed9

…ist_get_element

harrism added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 6, 2021

ttnghia approved these changes May 6, 2021

View reviewed changes

isVoid added 3 commits May 5, 2021 20:11

Fix tests

54c8374

More fix tests

65569b4

Scalar move column operator fix

6e25f96

nvdbaranec requested changes May 7, 2021

View reviewed changes

cpp/include/cudf/scalar/scalar_factories.hpp Outdated Show resolved Hide resolved

Update cpp/include/cudf/scalar/scalar_factories.hpp

aa38f7f

Co-authored-by: nvdbaranec <[email protected]>

nvdbaranec approved these changes May 7, 2021

View reviewed changes

rapids-bot bot merged commit bb62cf1 into rapidsai:branch-0.20 May 7, 2021

isVoid mentioned this pull request May 11, 2021

Preserve column hierarchy when getting NULL row from LIST column #8206

Merged

trxcllnt mentioned this pull request May 11, 2021

[FEA] Create ListSeries from JS Arrays, add ListSeries.getValue() rapidsai/node#187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `get_element` from LIST column #8071

Support `get_element` from LIST column #8071

isVoid commented Apr 26, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

harrism left a comment

ttnghia commented Apr 27, 2021 •

edited

Loading

isVoid commented Apr 27, 2021 •

edited

Loading

isVoid commented Apr 27, 2021

nvdbaranec left a comment

isVoid commented May 4, 2021

isVoid commented May 4, 2021

ttnghia commented May 6, 2021

kkraus14 commented May 7, 2021

Support get_element from LIST column #8071

Support get_element from LIST column #8071

Conversation

isVoid commented Apr 26, 2021 • edited Loading

codecov bot commented Apr 27, 2021 • edited Loading

Codecov Report

harrism left a comment

Choose a reason for hiding this comment

ttnghia commented Apr 27, 2021 • edited Loading

isVoid commented Apr 27, 2021 • edited Loading

isVoid commented Apr 27, 2021

nvdbaranec left a comment

Choose a reason for hiding this comment

isVoid commented May 4, 2021

isVoid commented May 4, 2021

ttnghia commented May 6, 2021

kkraus14 commented May 7, 2021

Support `get_element` from LIST column #8071

Support `get_element` from LIST column #8071

isVoid commented Apr 26, 2021 •

edited

Loading

codecov bot commented Apr 27, 2021 •

edited

Loading

ttnghia commented Apr 27, 2021 •

edited

Loading

isVoid commented Apr 27, 2021 •

edited

Loading