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

Added decimal writing for CSV writer #8296

Merged
merged 4 commits into from
May 21, 2021
Merged

Added decimal writing for CSV writer #8296

merged 4 commits into from
May 21, 2021

Conversation

kaatish
Copy link
Contributor

@kaatish kaatish commented May 20, 2021

Addresses #7110

column_to_strings_fn was specialized for fixed point type to enable support for csv writer. A test was added to validate output file created by csv writer for decimal type column.

Test for decimal writing in CSV writer
@kaatish kaatish added feature request New feature or request 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. cuIO cuIO issue non-breaking Non-breaking change labels May 20, 2021
@kaatish kaatish self-assigned this May 20, 2021
@kaatish kaatish requested a review from a team as a code owner May 20, 2021 14:01
@kaatish kaatish requested review from ttnghia, davidwendt, vuule and devavret and removed request for davidwendt May 20, 2021 14:01
Use EXPECT_EQ instead of ASSERT_EQ in test
@kaatish kaatish requested a review from davidwendt May 20, 2021 15:39
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.

Couple of suggestions to expand the test coverage.

cpp/tests/io/csv_test.cpp Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.06    #8296   +/-   ##
===============================================
  Coverage                ?   82.88%           
===============================================
  Files                   ?      105           
  Lines                   ?    17874           
  Branches                ?        0           
===============================================
  Hits                    ?    14814           
  Misses                  ?     3060           
  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 6b09253...b06282d. Read the comment docs.

@kaatish kaatish requested a review from vuule May 20, 2021 18:03
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 :)

cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
@vuule vuule added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels May 20, 2021
@vuule
Copy link
Contributor

vuule commented May 21, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit de579a5 into rapidsai:branch-21.06 May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge cuIO cuIO issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants