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

Add support for decimal128 #9483

Merged
merged 137 commits into from
Nov 16, 2021
Merged

Add support for decimal128 #9483

merged 137 commits into from
Nov 16, 2021

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Oct 20, 2021 •
edited by vuule
Loading

Fixes #9597
Fixes #9565

Previously, fixed_point along with decimal32 and decimal64 were added to support DecimalType (see #3556 for a list of major and minor PRs). With support for __int128_t now in CUDA 11.5, we can support decimal128. This PR enables decimal128.

choekstra added 30 commits July 20, 2021 02:07
Initial changes
c8a171c
More changes
afe6ec6
Small cleanup
43b615a
Small cleanup
ebedcad
Removal of device_storage_type_id, formatting and more
1d2e0b4
Formatting
2ea39fe
cudf::round support for __int128_t
606d6e3
Enable tests & fixes
ee70203
Missing changes
fd6157b
Scan, column_wrapper, orc, etc
d4506af
Binop changes
791e91c
detail::to_string
ad5fe35
Aggregation changes
7cc9db1
Small fix in fixed_point.hpp
5dd6874
Enable quantile
a89f958
Comment update
a16a2b8
REDUCTION_TEST working changes
e89a9ba
ROLLING_TEST changes
7ef28bf
Initial changes for STRINGS_TEST
7fd4ac4
STRINGS changes
016c35a
Clean up
dbd0504
Merge remote-tracking branch 'upstream/branch-21.10' into decimal128
9c764e6
std::is_same_v
bf34d20
is_integral & is_arithmetic
103a4db
Clean up
575fca7
Fixes / cleanup
8549753
DECIMAL128 custom reduction tests
22de55a
Another REDUCTION test
5b69c0c
numeric_limits / temporary cleanup
95667c8
More changes, 10+ files
825ab86
ajschmidt8
ajschmidt8 approved these changes Nov 12, 2021
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

@codereport codereport added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond and removed 2 - In Progress Currently a work in progress labels Nov 15, 2021
revans2
revans2 approved these changes Nov 15, 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 of things this looks good. I am still running through tests after the last upmerge, but they passed previously so I assume they will still. I'll post if I find any failure.

@codereport
Copy link
Contributor Author

From the java side of things this looks good. I am still running through tests after the last upmerge, but they passed previously so I assume they will still. I'll post if I find any failure.

@revans2 Is there a simple fix for the gpuCI/cudf/gpu/java CI build that is failing?

@revans2
Copy link
Contributor

revans2 commented Nov 15, 2021

From the java side of things this looks good. I am still running through tests after the last upmerge, but they passed previously so I assume they will still. I'll post if I find any failure.

@revans2 Is there a simple fix for the gpuCI/cudf/gpu/java CI build that is failing?

Not super simple. If you want me to send you a patch I can, but because it is not blocking the merge I just figured you would merge yours in and then we would merge in #9485 shortly after.

@codereport
Copy link
Contributor Author

From the java side of things this looks good. I am still running through tests after the last upmerge, but they passed previously so I assume they will still. I'll post if I find any failure.

@revans2 Is there a simple fix for the gpuCI/cudf/gpu/java CI build that is failing?

Not super simple. If you want me to send you a patch I can, but because it is not blocking the merge I just figured you would merge yours in and then we would merge in #9485 shortly after.

No patch necessary. We can just aim to merge one after the other.

@JohnZed
Copy link
Contributor

JohnZed commented Nov 15, 2021

@revans2 and @codereport sounds like a good plan (merging this with Java not passing then getting #9485) in right after.

harrism
harrism approved these changes Nov 16, 2021
View reviewed changes
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.

Epic undertaking, @codereport !

cpp/include/cudf/fixed_point/fixed_point.hpp Outdated Show resolved Hide resolved
v21.12 Release automation moved this from PR-WIP to PR-Reviewer approved Nov 16, 2021
@revans2
Copy link
Contributor

revans2 commented Nov 16, 2021

@codereport the style check has failed for one of the tests you added.

@codereport
Copy link
Contributor Author

@codereport the style check has failed for one of the tests you added.

Thanks! I had the fix locally but forgot to push it.

@jrhemstad jrhemstad added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Nov 16, 2021
@ajschmidt8 ajschmidt8 merged commit c1f20c7 into rapidsai:branch-21.12 Nov 16, 2021
v21.12 Release automation moved this from PR-Reviewer approved to Done Nov 16, 2021
ajschmidt8 pushed a commit that referenced this pull request Nov 17, 2021
@revans2
Java Support for Decimal 128 (#9485)
9aefbc2 
This depends on #9483

There may be a few more changes coming to this, but it should be fairly complete
rapids-bot bot pushed a commit that referenced this pull request Jan 18, 2022
@galipremsagar
Add support for decimal128 in cudf python (#9533)
8d7330f 
Resolves: #10031

Depends on #9483, #9986

Note: The CI for this PR is not going to pass until #9986 is admin-merged(Admin merge needed since #9986 requires this PR changes too). 

- [x] Introduced `Decimal128Dtype` and `Decimal128Column`.
- [x] Enabled python side support for the above both.
- [x] Enables complete support for `Decimal32Column` which is currently lacking.
- [x] Enabled orc writer to use decimal128.
- [x] Enabled parquet to read a decimal128 type.
- [x] Enabled Scalar support for `Decimal128Dtype`.
- [x] Covered all decimal types in `string` <-> `decimal` conversions.
- [x] **Made `Decimal128Dtype` the default type while reading in a Decimal Series or Scalar. User can specify to choose a specific decimal type by passing a `dtype`.** (Breaking)
- [x] **Fixed issues in the binop precision & scale calculation logic to correctly choose a decimal type.** (Breaking)
- [x] Fixed type metadata handling issues seen across APIs while making changes.
- [x] Added parametrizations for all missing `decimal32` tests.
- [x] Added parametrizations for `decimal128` along with existing decimal type-specific tests.

Authors:
  - GALI PREM SAGAR (https://github.com/galipremsagar)
  - Robert (Bobby) Evans (https://github.com/revans2)
  - Conor Hoekstra (https://github.com/codereport)

Approvers:
  - Devavret Makkar (https://github.com/devavret)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #9533
@jbrennan333 jbrennan333 mentioned this pull request Apr 4, 2022
Closed
@codereport codereport mentioned this pull request Aug 21, 2023
Closed
@vyasr vyasr mentioned this pull request May 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertmaynard robertmaynard robertmaynard left review comments

@revans2 revans2 revans2 approved these changes

@vuule vuule vuule left review comments

@harrism harrism harrism approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@shwina shwina Awaiting requested review from shwina shwina is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@skirui-source skirui-source Awaiting requested review from skirui-source skirui-source is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees

@codereport codereport

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 Python Affects Python cuDF API.
Projects
No open projects
v21.12 Release
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Cast Decimal64 to Decimal128 is really slow [FEA] Decimal128 support in ORC reader/writer
9 participants
@codereport @ajschmidt8 @revans2 @robertmaynard @sperlingxx @vuule @JohnZed @harrism @jrhemstad