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 Decimal DIV changes in cudf [skip ci] #7527

Merged
merged 5 commits into from
Mar 16, 2021
Merged

Support Decimal DIV changes in cudf [skip ci] #7527

merged 5 commits into from
Mar 16, 2021

Conversation

razajafri
Copy link
Contributor

@codereport is making changes to the way DIV will behave for fixed-point types #7435. This PR contains Java changes to support those changes.

Note: This is a draft until #7435 is merged

@github-actions github-actions bot added the Java Affects Java cuDF API. label Mar 7, 2021
@razajafri razajafri added 4 - Needs cuDF (Java) Reviewer non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS 3 - Ready for Review Ready for review by team Java Affects Java cuDF API. improvement Improvement / enhancement to an existing function and removed Java Affects Java cuDF API. labels Mar 7, 2021
@razajafri razajafri marked this pull request as ready for review March 12, 2021 23:47
@razajafri razajafri requested a review from a team as a code owner March 12, 2021 23:47
@razajafri
Copy link
Contributor Author

razajafri commented Mar 12, 2021 •
edited
Loading

Now that #7435 is merged. This will break Decimal div

mythrocks
mythrocks approved these changes Mar 13, 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.

A couple of minor nitpicks, but looks good to me.

@mythrocks mythrocks self-requested a review March 13, 2021 00:11
mythrocks
mythrocks requested changes Mar 13, 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.

(Accidentally approved, earlier.)
Minor nitpicks: Javadoc + typo in method name.

gerashegalov
gerashegalov reviewed Mar 15, 2021
View reviewed changes
Copy link
Contributor

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

@razajafri can you merge the parent branch-0.19 to consume binary_operation_fixed_point_scale into your PR (, and address the previous comments on the PR?

Otherwise, LGTM

@github-actions github-actions bot added the CMake CMake build issue label Mar 15, 2021
@razajafri
Copy link
Contributor Author

Addressed comments. PTAL

@jlowe do you have any more concerns? I added a small change to the CMakeLists.txt for not overriding the CUDF_CPP_BUILD_DIR if its set in the environment. This is needed to build the bindings within the compose docker. Let me know if you want me to spin this change into a separate PR.

jlowe
jlowe reviewed Mar 15, 2021
View reviewed changes
java/src/main/native/CMakeLists.txt Outdated Show resolved Hide resolved
java/src/main/java/ai/rapids/cudf/ColumnView.java Outdated Show resolved Hide resolved
@razajafri
Copy link
Contributor Author

@jlowe PTAL.

@jlowe jlowe removed the CMake CMake build issue label Mar 15, 2021
gerashegalov
gerashegalov approved these changes Mar 16, 2021
View reviewed changes
Copy link
Contributor

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

LGTM

@jlowe
Copy link
Member

jlowe commented Mar 16, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 2b2c0d2 into rapidsai:branch-0.19 Mar 16, 2021
@razajafri razajafri mentioned this pull request Mar 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mythrocks mythrocks mythrocks approved these changes

@gerashegalov gerashegalov gerashegalov approved these changes

@jlowe jlowe jlowe approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. 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.
4 participants
@razajafri @jlowe @gerashegalov @mythrocks