[REVIEW] Initial support for struct columns #5807

mythrocks · 2020-07-30T06:18:23Z

Closes #5700.

This PR adds rudimentary support for struct columns in CUDF, the behaviour of which is described here:
#5700

GPUtester · 2020-07-30T06:19:54Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

cpp/src/structs/structs_column_factories.cu

Changed how null-masks are retrrieved from child columns.

Removed redundant error checks. Also, simplified for loops.

Added tests for List<Struct>. Reordered some struct tests. Removed unnecessary reference_wrapper helper functions.

Removed dead code. Fixed typo in variable name.

cpp/include/cudf/structs/structs_column_view.hpp

cpp/include/cudf/structs/struct_view.hpp

cpp/include/cudf/column/column_view.hpp

codecov · 2020-08-06T21:31:02Z

Codecov Report

Merging #5807 into branch-0.15 will decrease coverage by 4.07%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           branch-0.15    #5807      +/-   ##
===============================================
- Coverage        88.65%   84.57%   -4.08%     
===============================================
  Files               57       81      +24     
  Lines            10945    13852    +2907     
===============================================
+ Hits              9703    11715    +2012     
- Misses            1242     2137     +895

Impacted Files	Coverage Δ
python/cudf/cudf/core/dtypes.py	`86.84% <0.00%> (-10.09%)`	⬇️
python/cudf/cudf/core/abc.py	`91.48% <0.00%> (-2.96%)`	⬇️
python/cudf/cudf/core/frame.py	`89.76% <0.00%> (-1.42%)`	⬇️
python/cudf/cudf/utils/ioutils.py	`86.13% <0.00%> (-1.21%)`	⬇️
python/dask_cudf/dask_cudf/sorting.py	`93.12% <0.00%> (-0.99%)`	⬇️
python/cudf/cudf/core/indexing.py	`95.22% <0.00%> (-0.68%)`	⬇️
python/cudf/cudf/utils/dtypes.py	`86.03% <0.00%> (-0.40%)`	⬇️
python/cudf/cudf/core/column/categorical.py	`93.61% <0.00%> (-0.10%)`	⬇️
python/cudf/cudf/core/groupby/groupby.py	`93.18% <0.00%> (-0.10%)`	⬇️
python/cudf/cudf/core/column/column.py	`87.36% <0.00%> (-0.01%)`	⬇️
... and 62 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 5c57bd5...1c0ab90. Read the comment docs.

cpp/src/structs/structs_column_factories.cu

cpp/tests/utilities/column_wrapper.hpp

1. Added documentation for struct_column_wrapper. 2. Removed unnecessary std::move for child null masks.

mythrocks · 2020-08-10T23:35:10Z

Super! Thank you for reviewing, @raydouglass, @ajschmidt8, @jrhemstad.

~~Might this PR please be merged?~~
I'll request for another C++ review.

mythrocks · 2020-08-11T15:30:46Z

Thanks for picking up the review, @nvdbaranec. :]

nvdbaranec

I think it would be good to have more tests involving empty columns, particularly in the nested list/struct case. Also some tests that bounce between list<struct<list<struct< etc (with validity and nulls, etc).

cpp/src/search/search.cu

cpp/src/scalar/scalar_factories.cpp

cpp/src/structs/structs_column_factories.cu

cpp/src/structs/structs_column_view.cu

cpp/tests/structs/structs_column_tests.cu

nvdbaranec · 2020-08-11T19:43:28Z

cpp/tests/structs/structs_column_tests.cu

+  cudf::test::expect_columns_equivalent(expected_unchanged_struct_col,
+                                        cudf::lists_column_view(*list_col).child());
+
+#ifndef NDEBUG


@jrhemstad Is there a cudf standard for debug stuff like this? This is generally something I like to do but have avoided so far because it doesn't seem to be used in cudf at large.

I took my cues from rolling_test, for the printing. (Although there, it was under a #if 0.)

Considering that the cudf debug build doesn't even succeed currently (if you got it to build, I'd love to hear about it), I'm guessing you didn't compile this code?

I'm guessing you didn't compile this code?

Not in debug mode, but I do use the prints. I comment out the #ifndef NDEBUG, when I'm debugging. :/

I left this in for when I figure out debugging, proper.

I'm happy to remove these prints, if that's preferable.

mythrocks · 2020-08-11T20:58:54Z

Also some tests that bounce between list<struct<list<struct< etc (with validity and nulls, etc).

The reason I don't have deeper nested tests is:

It is wordy. Both the composition of the test columns and the result checks are very verbose at the moment. I'm afraid I don't have support for literals yet.
Checking results for list<struct> and struct<list> has proven difficult because of the equality/equivalence issue listed in [QST] expect_columns_equal() vs expect_columns_equivalent() #5867. :/

1. Added tests for deeper nesting. 2. Fixed license headers. 3. Added convenience methods to structs_column_wrapper.

mythrocks · 2020-08-12T04:19:18Z

The reason I don't have deeper nested tests is:

I have added tests for list<struct<list<numeric>>> and struct<list<struct<numeric>>>.

mythrocks · 2020-08-13T17:00:22Z

Super. Thanks for reviewing, @nvdbaranec.
I'll get to work on the gather() piece now.

[WIP] [struct] Initial commit

0683c40

mythrocks requested review from a team as code owners July 30, 2020 06:18

mythrocks requested review from trevorsm7 and karthikeyann July 30, 2020 06:18

mythrocks self-assigned this Jul 30, 2020

mythrocks added the feature request New feature or request label Jul 30, 2020

jrhemstad requested changes Jul 30, 2020

View reviewed changes

cpp/src/structs/structs_column_factories.cu Outdated Show resolved Hide resolved

cpp/src/structs/structs_column_factories.cu Outdated Show resolved Hide resolved

mythrocks added 2 commits July 30, 2020 16:28

[WIP] [struct] Review comments

8ed38e3

Changed how null-masks are retrrieved from child columns.

Merge remote-tracking branch 'origin/branch-0.15' into structs

673765b

mythrocks changed the title ~~[WIP] Tentative support for struct columns~~ [WIP] Initial support for struct columns Jul 31, 2020

mythrocks added 11 commits July 31, 2020 16:30

[struct] Added struct-support issue to CHANGELOG.md

ec69d74

[struct] Switched to in-place bitmask_and()

0261617

[struct] Minor cleanup.

1577732

Removed redundant error checks. Also, simplified for loops.

[struct] Added test for null-mask propagation (For non-null structs

7e9d660

[struct] More tests and cleanup.

7e197d6

Added tests for List<Struct>. Reordered some struct tests. Removed unnecessary reference_wrapper helper functions.

Merge remote-tracking branch 'origin/branch-0.15' into structs

8391618

[struct] Added structs to lists' tests.

d2216ce

[struct] Added structs to lists' tests.

1d6b1ea

Removed dead code. Fixed typo in variable name.

[struct] Added doc for struct factory method.

1037904

[struct] clang-format

be7de05

Merge remote-tracking branch 'origin/branch-0.15' into structs

3bdbe50

mythrocks changed the title ~~[WIP] Initial support for struct columns~~ Initial support for struct columns Aug 6, 2020

mythrocks mentioned this pull request Aug 6, 2020

[QST] expect_columns_equal() vs expect_columns_equivalent() #5867

Closed

[struct] Added struct headers to meta.yaml

b4a879f

mythrocks requested a review from a team as a code owner August 6, 2020 18:57

raydouglass approved these changes Aug 6, 2020

View reviewed changes

mythrocks commented Aug 6, 2020

View reviewed changes

cpp/include/cudf/structs/structs_column_view.hpp Show resolved Hide resolved

cpp/include/cudf/structs/struct_view.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/column/column_view.hpp Outdated Show resolved Hide resolved

mythrocks added 3 commits August 7, 2020 10:40

[struct] Switch tests to check column equivalence

db85253

[struct] clang-format changes in struct tests

a48a5b7

[struct] Corrected nullmask size check in struct factory

671ec1c

ajschmidt8 approved these changes Aug 10, 2020

View reviewed changes

jrhemstad reviewed Aug 10, 2020

View reviewed changes

cpp/src/structs/structs_column_factories.cu Outdated Show resolved Hide resolved

cpp/tests/utilities/column_wrapper.hpp Show resolved Hide resolved

mythrocks added 2 commits August 10, 2020 12:25

[struct] Incorporated review changes.

6b9082e

1. Added documentation for struct_column_wrapper. 2. Removed unnecessary std::move for child null masks.

Merge remote-tracking branch 'origin/branch-0.15' into structs

d1d7722

mythrocks requested a review from jrhemstad August 10, 2020 22:44

mythrocks mentioned this pull request Aug 10, 2020

[FEA] cudf::gather for Struct columns #5912

Closed

jrhemstad approved these changes Aug 10, 2020

View reviewed changes

mythrocks requested review from nvdbaranec and removed request for trevorsm7 and karthikeyann August 11, 2020 15:30

nvdbaranec requested changes Aug 11, 2020

View reviewed changes

mythrocks added 2 commits August 11, 2020 14:04

Merge remote-tracking branch 'origin/branch-0.15' into structs

5ca14cb

[struct] More review-related corrections

bb93f7a

1. Added tests for deeper nesting. 2. Fixed license headers. 3. Added convenience methods to structs_column_wrapper.

mythrocks added 3 commits August 11, 2020 21:20

[struct] clang-format again.

906e04c

Merge remote-tracking branch 'origin/branch-0.15' into structs

f6257b3

[struct] Added tests for empty columns.

3380822

nvdbaranec self-requested a review August 12, 2020 23:50

nvdbaranec approved these changes Aug 12, 2020

View reviewed changes

Merge branch 'branch-0.15' into structs

1c0ab90

mythrocks merged commit aa90152 into rapidsai:branch-0.15 Aug 13, 2020

mythrocks deleted the structs branch August 13, 2020 17:00

mythrocks mentioned this pull request Aug 25, 2020

[WIP] Changes to expect_columns_equivalent() for supporting lists #6091

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Initial support for struct columns #5807

[REVIEW] Initial support for struct columns #5807

mythrocks commented Jul 30, 2020

GPUtester commented Jul 30, 2020

codecov bot commented Aug 6, 2020 •

edited

Loading

mythrocks commented Aug 10, 2020 •

edited

Loading

mythrocks commented Aug 11, 2020

nvdbaranec left a comment

nvdbaranec Aug 11, 2020

mythrocks Aug 11, 2020

harrism Aug 12, 2020

mythrocks Aug 12, 2020 •

edited

Loading

mythrocks Aug 12, 2020

mythrocks commented Aug 11, 2020

mythrocks commented Aug 12, 2020

mythrocks commented Aug 13, 2020

[REVIEW] Initial support for struct columns #5807

[REVIEW] Initial support for struct columns #5807

Conversation

mythrocks commented Jul 30, 2020

GPUtester commented Jul 30, 2020

codecov bot commented Aug 6, 2020 • edited Loading

Codecov Report

mythrocks commented Aug 10, 2020 • edited Loading

mythrocks commented Aug 11, 2020

nvdbaranec left a comment

Choose a reason for hiding this comment

nvdbaranec Aug 11, 2020

Choose a reason for hiding this comment

mythrocks Aug 11, 2020

Choose a reason for hiding this comment

harrism Aug 12, 2020

Choose a reason for hiding this comment

mythrocks Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

mythrocks Aug 12, 2020

Choose a reason for hiding this comment

mythrocks commented Aug 11, 2020

mythrocks commented Aug 12, 2020

mythrocks commented Aug 13, 2020

codecov bot commented Aug 6, 2020 •

edited

Loading

mythrocks commented Aug 10, 2020 •

edited

Loading

mythrocks Aug 12, 2020 •

edited

Loading