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

Support for struct scalars. #8220

Merged
merged 6 commits into from
May 18, 2021
Merged

Support for struct scalars. #8220

merged 6 commits into from
May 18, 2021

Conversation

nvdbaranec
Copy link
Contributor

@nvdbaranec nvdbaranec commented May 12, 2021 •
edited
Loading

Closes: #7790

Creation of struct scalars.

Also in this PR:

  • support for struct_scalar in make_column_from_scalar
  • Refactored superimpose_parent_nullmask to be exposed as structs::detail::superimpose_parent_nulls in src/structs/utilities.hpp

@nvdbaranec nvdbaranec added feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change labels May 12, 2021
@nvdbaranec nvdbaranec requested review from a team as code owners May 12, 2021 00:03
@nvdbaranec nvdbaranec requested review from robertmaynard and vuule May 12, 2021 00:03
@github-actions github-actions bot added the CMake CMake build issue label May 12, 2021
@codecov
Copy link

codecov bot commented May 12, 2021 •
edited
Loading

Codecov Report

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

❗ Current head 0e882b4 differs from pull request most recent head a242363. Consider uploading reports for the commit a242363 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #8220   +/-   ##
===============================================
  Coverage                ?   82.85%           
===============================================
  Files                   ?      105           
  Lines                   ?    17891           
  Branches                ?        0           
===============================================
  Hits                    ?    14823           
  Misses                  ?     3068           
  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 8406522...a242363. Read the comment docs.

robertmaynard
robertmaynard requested changes May 12, 2021
View reviewed changes
cpp/CMakeLists.txt Show resolved Hide resolved
vuule
vuule requested changes May 12, 2021
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Bunch of very minor suggestions, looks great otherwise.

cpp/src/structs/utilities.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar_factories.hpp Outdated Show resolved Hide resolved
cpp/tests/scalar/scalar_test.cpp Outdated Show resolved Hide resolved
cpp/tests/scalar/scalar_test.cpp Outdated Show resolved Hide resolved
cpp/include/cudf/scalar/scalar.hpp Outdated Show resolved Hide resolved
cpp/src/scalar/scalar.cpp Outdated Show resolved Hide resolved
cpp/src/scalar/scalar.cpp Outdated Show resolved Hide resolved
cpp/src/scalar/scalar_factories.cpp Outdated Show resolved Hide resolved
cpp/tests/column/factories_test.cpp Outdated Show resolved Hide resolved
cpp/tests/scalar/factories_test.cpp Outdated Show resolved Hide resolved
@davidwendt
Copy link
Contributor

The cpp/src/structs/utilities.cu does not appear to have any device code in it.
I did a quick test and found this file could be renamed to .cpp instead.

@kkraus14
Copy link
Collaborator

cc @brandon-b-miller for visibility since you're working on Python List Scalar plumbing already.

@nvdbaranec
Copy link
Contributor Author

The cpp/src/structs/utilities.cu does not appear to have any device code in it.
I did a quick test and found this file could be renamed to .cpp instead.

Done.

@nvdbaranec nvdbaranec requested review from robertmaynard and vuule May 14, 2021 16:47
vuule
vuule approved these changes May 14, 2021
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Couple more unused local variables, otherwise LGTM

cpp/tests/scalar/factories_test.cpp Outdated Show resolved Hide resolved
cpp/tests/scalar/factories_test.cpp Outdated Show resolved Hide resolved
robertmaynard
robertmaynard approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment

Choose a reason for hiding this comment

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

Approving cmake-codeowner file changes

@nvdbaranec
Copy link
Contributor Author

rerun tests

@nvdbaranec
Copy link
Contributor Author

rerun tests

1 similar comment
@nvdbaranec
Copy link
Contributor Author

rerun tests

@nvdbaranec nvdbaranec changed the title Add support for struct_scalar Support for struct scalars. May 18, 2021
@nvdbaranec
Copy link
Contributor Author

rerun tests

@nvdbaranec
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 414e9bb into rapidsai:branch-21.06 May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt left review comments

@vuule vuule vuule approved these changes

@ttnghia ttnghia ttnghia approved these changes

@robertmaynard robertmaynard robertmaynard approved these changes

Assignees
No one assigned
Labels
CMake CMake build issue 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] Add scalar support for struct
6 participants
@nvdbaranec @davidwendt @kkraus14 @robertmaynard @ttnghia @vuule