Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nested types for nth_element reduction #9043

Merged
merged 10 commits into from
Aug 24, 2021

Conversation

sperlingxx
Copy link
Contributor

Closes #8967

Current PR supported the construction of default scalar on nested types (LIST_TYPE and STRUCT_TYPE) for reduction, in order to support nested types for nth_element reduction.

@sperlingxx sperlingxx requested a review from a team as a code owner August 16, 2021 12:12
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 16, 2021
@sperlingxx sperlingxx added non-breaking Non-breaking change feature request New feature or request labels Aug 16, 2021
if (col.size() <= col.null_count()) return result;
// Returns default scalar if input column is non-valid. In terms of nested columns, we need to
// handcraft the default scalar with input column.
if (col.size() <= col.null_count()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like we need something like a make_empty_scalar_like(column_view)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. And I created the method make_empty_scalar_like based on the above code.

Signed-off-by: sperlingxx <[email protected]>
@codecov
Copy link

codecov bot commented Aug 17, 2021

Codecov Report

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

❗ Current head 8d496c1 differs from pull request most recent head 608565a. Consider uploading reports for the commit 608565a to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #9043   +/-   ##
===============================================
  Coverage                ?   10.71%           
===============================================
  Files                   ?      114           
  Lines                   ?    19090           
  Branches                ?        0           
===============================================
  Hits                    ?     2046           
  Misses                  ?    17044           
  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 eb85d77...608565a. Read the comment docs.

@sperlingxx sperlingxx requested a review from ttnghia August 18, 2021 04:38
result->set_valid_async(false, stream);
break;
case type_id::STRUCT:
// Struct scalar inputs must have exactly 1 row.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have "at least one row", I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, you don't have to have your own CUDF_EXPECT because rows == 1 will be handled in the scalar constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, then you should use make_struct_scalar too :|

Co-authored-by: David Wendt <[email protected]>
Co-authored-by: Nghia Truong <[email protected]>
cpp/src/scalar/scalar_factories.cpp Outdated Show resolved Hide resolved
case type_id::STRUCT:
// Struct scalar inputs must have exactly 1 row.
CUDF_EXPECTS(!column.is_empty(), "Can not create empty struct scalar");
result = detail::get_element(column, 1, stream, mr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index=1 ? (should be 0 right?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about result = make_struct_scalar(column, stream, mr)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make_struct_scalar only accepts table_view or host_span<column_view>. In addition, it assumes the size of input data == 1 (rather than slicing them to 1).

Copy link
Contributor

@ttnghia ttnghia Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, expecting full_size() == 1 (not sliced_size() == 1) should be wrong!
OK get it. So you are just slicing the input column (get one row) from it and don't care how many rows it has.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index=1 ? (should be 0 right?)

Yes, I corrected it. Thanks!

@ttnghia
Copy link
Contributor

ttnghia commented Aug 20, 2021

Rerun tests.

Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@sperlingxx
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d7a05dc into rapidsai:branch-21.10 Aug 24, 2021
@sperlingxx sperlingxx deleted the default_nested_scalar branch August 24, 2021 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] add nested struct support for nth reduction and group by aggregation
5 participants