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

Decimal support csv reader #8511

Merged
merged 12 commits into from
Jun 23, 2021
Merged

Decimal support csv reader #8511

merged 12 commits into from
Jun 23, 2021

Conversation

elstehle
Copy link
Contributor

This PR adds support for decimal types in libcudf.

Specifically, the PR:

  1. expands the C++ API to allow passing std::vector<data_type> (partially addresses [FEA] CSV and JSON readers: Pass column data types as objects instead of strings #8240)
  2. it moves the logic for parsing decimal types to common ground, such that it can be used for both string_to_decimal conversion and for the CSV reader.
  3. adds support to the CSV reader to also accept decimal as a target type (partially addresses [FEA] support read/write csv with fixed-point decimal data #7110)

@elstehle elstehle requested a review from a team as a code owner June 14, 2021 15:41
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Jun 14, 2021
@elstehle elstehle added cuIO cuIO issue feature request New feature or request non-breaking Non-breaking change labels Jun 14, 2021
@elstehle elstehle requested a review from vuule June 14, 2021 15:43
@codecov
Copy link

codecov bot commented Jun 14, 2021 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8511   +/-   ##
===============================================
  Coverage                ?   83.00%           
===============================================
  Files                   ?      109           
  Lines                   ?    18215           
  Branches                ?        0           
===============================================
  Hits                    ?    15119           
  Misses                  ?     3096           
  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 8aceab0...4feacb7. Read the comment docs.

vuule
vuule requested changes Jun 14, 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 for the most part, a few suggestions/questions:

cpp/src/io/csv/csv_gpu.cu Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Show resolved Hide resolved
cpp/include/cudf/io/csv.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/strings/detail/convert/fixed_point.cuh Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Show resolved Hide resolved
@elstehle elstehle requested a review from davidwendt June 15, 2021 04:41
devavret
devavret approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

Agree with deprecating old API. Review only suggestions.

cpp/src/io/csv/csv_gpu.cu Outdated Show resolved Hide resolved
cpp/src/io/csv/csv_gpu.cu Outdated Show resolved Hide resolved
@vuule
Copy link
Contributor

vuule commented Jun 22, 2021

rerun tests

@vuule vuule self-requested a review June 22, 2021 20:38
vuule
vuule approved these changes Jun 22, 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.

🔥

@elstehle @davidwendt
east const
a6fd2d2 
Co-authored-by: David Wendt <[email protected]>
davidwendt
davidwendt approved these changes Jun 22, 2021
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

I only really reviewed the strings changes. The rest of it looks good to me.

@elstehle
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 788bddd into rapidsai:branch-21.08 Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@vuule vuule vuule approved these changes

@devavret devavret devavret approved these changes

@karthikeyann karthikeyann Awaiting requested review from karthikeyann

Assignees
No one assigned
Labels
cuIO cuIO issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elstehle @vuule @devavret @davidwendt