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 scalar months in add_calendrical_months, extends API to INT32 support #8991

Merged
merged 16 commits into from
Sep 15, 2021
Merged

Support scalar months in add_calendrical_months, extends API to INT32 support #8991

merged 16 commits into from
Sep 15, 2021

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Aug 6, 2021 •
edited
Loading

Closes #6990

This PR adds support for scalar months for add_calendrical_months API. The behavior is similar to the column version of the API, as if applying the month value from a single row of the months column to the input column. This PR also refactors add_calendrical_months_functor to accept iterator based months argument.

Closes #8865
Also adds support to INT32 type for both column and scalar version of the API.

@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Aug 6, 2021
@isVoid isVoid self-assigned this Aug 6, 2021
@isVoid isVoid marked this pull request as ready for review August 7, 2021 00:58
@isVoid isVoid requested a review from a team as a code owner August 7, 2021 00:58
@isVoid isVoid requested review from trxcllnt and vuule August 7, 2021 00:58
@isVoid isVoid added feature request New feature or request non-breaking Non-breaking change labels Aug 7, 2021
@codecov
Copy link

codecov bot commented Aug 7, 2021 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-21.10   #8991   +/-   ##
==============================================
  Coverage                ?   9.39%           
==============================================
  Files                   ?     115           
  Lines                   ?   24322           
  Branches                ?       0           
==============================================
  Hits                    ?    2285           
  Misses                  ?   22037           
  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 31b731e...40165cd. Read the comment docs.

revans2
revans2 approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

From the java side this looks great. It is a really good step towards what we need.

vuule
vuule reviewed Aug 9, 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.

Looks good, just got a small comment and a question.

cpp/tests/datetime/datetime_ops_test.cpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
@isVoid isVoid changed the title Support scalar months in add_calendrical_months Support scalar months in add_calendrical_months, extends API to INT32 support Aug 11, 2021
vuule
vuule approved these changes Aug 11, 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.

doc string needs to be updated, otherwise looks good.

cpp/include/cudf/datetime.hpp Show resolved Hide resolved
ttnghia
ttnghia requested changes Aug 12, 2021
View reviewed changes
cpp/include/cudf/datetime.hpp Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Show resolved Hide resolved
cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/src/datetime/datetime_ops.cu Show resolved Hide resolved
@isVoid isVoid requested a review from ttnghia August 13, 2021 18:11
ttnghia
ttnghia reviewed Aug 18, 2021
View reviewed changes
Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

This looks good, except one problem that Jake has raised: Should we support all types of int?

@isVoid isVoid added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Aug 24, 2021
@galipremsagar
Copy link
Contributor

rerun tests

@galipremsagar
Copy link
Contributor

@isVoid is this good to merge now?

@isVoid
Copy link
Contributor Author

isVoid commented Sep 15, 2021

@galipremsagar I think so. Thanks for tracking!

@isVoid
Copy link
Contributor Author

isVoid commented Sep 15, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 0cf2c5f into rapidsai:branch-21.10 Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhemstad jrhemstad jrhemstad left review comments

@vuule vuule vuule approved these changes

@ttnghia ttnghia ttnghia approved these changes

@revans2 revans2 revans2 approved these changes

@trxcllnt trxcllnt Awaiting requested review from trxcllnt

Assignees

@isVoid isVoid

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
6 participants
@isVoid @galipremsagar @revans2 @ttnghia @jrhemstad @vuule