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

[REVIEW] Define and implement new datetime_ops APIs #3201

Merged
merged 46 commits into from
Nov 22, 2019

Conversation

trxcllnt
Copy link
Contributor

@trxcllnt trxcllnt commented Oct 23, 2019

Do not merge until libcudacxx/pull/5 and libcxx/pull/17 are resolved.

This PR implements datetime_ops.cu for the new libcudf++ column using the new C++20 calendar functions in libcudacxx 🎉.

I made a few changes to column_view and column_device_view begin/end iterators which I found necessary, but feel free to suggest alternatives if you disagree.

Related issue: #2935

@trxcllnt trxcllnt added the 0 - Blocked Cannot progress due to external reasons label Oct 23, 2019
@trxcllnt trxcllnt requested review from a team as code owners October 23, 2019 20:29
Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

I'm over the moon at how clean this implementation is thanks to libcu++. 😍

cpp/src/datetime/datetime_ops.cu Outdated Show resolved Hide resolved
cpp/tests/datetime/datetime_ops_test.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 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/tests/utilities/timestamp_utilities.cuh Outdated Show resolved Hide resolved
cpp/tests/datetime/datetime_ops_test.cu Outdated Show resolved Hide resolved
cpp/tests/datetime/datetime_ops_test.cu Outdated Show resolved Hide resolved
@trxcllnt
Copy link
Contributor Author

rerun tests

@harrism harrism requested a review from jrhemstad November 17, 2019 21:30
@trxcllnt trxcllnt removed the 2 - In Progress Currently a work in progress label Nov 18, 2019
cpp/include/cudf/column/column_device_view.cuh Outdated 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/utilities/traits.hpp Show resolved Hide resolved
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

One thing missed.

cpp/include/cudf/datetime.hpp Outdated Show resolved Hide resolved
@jrhemstad jrhemstad requested a review from harrism November 21, 2019 18:45
@trxcllnt
Copy link
Contributor Author

After this is merged we'll need folks to run this to update the libcu++ submodule correctly:

git submodule sync --recursive && git submodule update --init --recursive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants