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.

Sign up for GitHub

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

Jump to bottom

Nested scalar support for copy if else #8588

Merged
merged 43 commits into from
Jul 20, 2021
Merged

Nested scalar support for copy if else #8588

merged 43 commits into from
Jul 20, 2021

Conversation

gerashegalov
Copy link
Contributor

@gerashegalov gerashegalov commented Jun 22, 2021 •
edited
Loading

Uses scalar-vector-based scatter API to provide support for copy_if_else involving scalar columns.

Other changes:

  • removes some dead code
  • refactoring into overloaded functions

Closes #8361, depends on #8630, #8666

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 22, 2021
@gerashegalov gerashegalov self-assigned this Jun 23, 2021
@gerashegalov gerashegalov linked an issue Jun 23, 2021 that may be closed by this pull request
[FEA] copy_if_else support for nested scalar values #8361
Closed
@codecov
Copy link

codecov bot commented Jun 24, 2021 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8588   +/-   ##
===============================================
  Coverage                ?   10.51%           
===============================================
  Files                   ?      116           
  Lines                   ?    18957           
  Branches                ?        0           
===============================================
  Hits                    ?     1993           
  Misses                  ?    16964           
  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 8bd0dfe...40cfcf4. Read the comment docs.

@gerashegalov gerashegalov changed the title [WIP] struct scalar support for copy if else [WIP] Nested scalar support for copy if else Jun 24, 2021
@gerashegalov
Copy link
Contributor Author

gerashegalov commented Jul 2, 2021 •
edited
Loading

LGTM
Maybe in the future, we could implement this by leveraging the API scatter accepting a vector of scalars to avoid expanding a scalar to a column.

+1 I would love to see a follow on issue filed for that.

I agree, I reflected in the PR description that this is a naive implementation. Depending on whether this iteration of the PR is approved, I'll file a follow-up issue on improving it.

@nvdbaranec , @mythrocks please take a look, too

prototyped this in gerashegalov#1 but it will need more work for structs which is going in with #8630. So I think it should be definitely a follow-up PR.

@gerashegalov gerashegalov mentioned this pull request Jul 7, 2021
Merged
@gerashegalov
Copy link
Contributor Author

LGTM
Maybe in the future, we could implement this by leveraging the API scatter accepting a vector of scalars to avoid expanding a scalar to a column.

+1 I would love to see a follow on issue filed for that.

#8630 went in. So I folded gerashegalov#1 into this PR that uses scalar-vector-based API

@firestarman
Copy link
Contributor

rerun tests

mythrocks
mythrocks reviewed Jul 13, 2021
View reviewed changes
cpp/src/copying/copy.cu Outdated Show resolved Hide resolved
cpp/src/copying/copy.cu Show resolved Hide resolved
nvdbaranec
nvdbaranec requested changes Jul 15, 2021
View reviewed changes
cpp/src/copying/copy.cu Outdated Show resolved Hide resolved
cpp/src/copying/copy.cu Show resolved Hide resolved
cpp/src/copying/copy.cu Show resolved Hide resolved
cpp/src/copying/copy.cu Show resolved Hide resolved
nvdbaranec
nvdbaranec approved these changes Jul 15, 2021
View reviewed changes
cpp/src/copying/copy.cu Show resolved Hide resolved
mythrocks
mythrocks approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@gerashegalov
Copy link
Contributor Author

rerun tests

@gerashegalov
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7ee347c into rapidsai:branch-21.08 Jul 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvdbaranec nvdbaranec nvdbaranec approved these changes

@mythrocks mythrocks mythrocks approved these changes

@firestarman firestarman Awaiting requested review from firestarman

Assignees

@gerashegalov gerashegalov

Labels
3 - Ready for Review Ready for review by team feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] copy_if_else support for nested scalar values
5 participants
@gerashegalov @firestarman @revans2 @mythrocks @nvdbaranec