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

Simplify Python implementations of to_arrow/from_arrow for decimals #8430

Closed
wants to merge 2 commits into from
Closed

Simplify Python implementations of to_arrow/from_arrow for decimals #8430

wants to merge 2 commits into from

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Jun 2, 2021

#7609 added libcudf implementations of these functions, so we can simplify the Python greatly.

@shwina shwina requested a review from a team as a code owner June 2, 2021 21:23
@shwina shwina requested review from marlenezw and skirui-source June 2, 2021 21:23
@github-actions github-actions bot added the Python Affects Python cuDF API. label Jun 2, 2021
@shwina shwina added tech debt non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jun 2, 2021
marlenezw
marlenezw reviewed Jun 4, 2021
View reviewed changes
Copy link
Contributor

@marlenezw marlenezw 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 to me! The Dask test failures seem unrelated to the changes, so maybe re-running the tests might help? Really great how much cleaner this makes the code, and wondering if this will work for other cases too :D

@shwina shwina changed the base branch from branch-21.08 to branch-21.10 July 22, 2021 14:26
@shwina
Copy link
Contributor Author

shwina commented Jul 22, 2021 •
edited
Loading

Retargeting to 21.10 as there's more work to be done here. We now have 32-bit decimals in Python. libcudf doesn't yet have to/from_arrow implementations for 32-bit decimal types and this PR depends on that functionality.

@vyasr
Copy link
Contributor

vyasr commented Sep 23, 2021

@shwina @skirui-source are the necessary libcudf changes ready for this? Should we move to 21.12?

@shwina
Copy link
Contributor Author

shwina commented Sep 23, 2021

Pushed to 21.12, as this is going to need some more work on the libcudf side.

@skirui-source skirui-source marked this pull request as draft October 13, 2021 23:13
@github-actions
Copy link

This PR has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this PR if it is no longer required. Otherwise, please respond with a comment indicating any updates.

@shwina
Copy link
Contributor Author

shwina commented Jan 12, 2022

I'm closing this PR as it has gone stale. The blocker for this PR has been that libcudf has no concept of precision, and being able to convey that metadata about the column to libcudf was essential for correctness, although now the details are a bit hazy for me. For now, keeping the Python implementations seems fine.

@shwina shwina closed this Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marlenezw marlenezw marlenezw approved these changes

@skirui-source skirui-source Awaiting requested review from skirui-source

Assignees
No one assigned
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shwina @vyasr @marlenezw