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

BUG FIX: CSV Writer ignores the header parameter when no metadata is provided #8740

Merged
merged 16 commits into from
Oct 14, 2021
Merged

BUG FIX: CSV Writer ignores the header parameter when no metadata is provided #8740

merged 16 commits into from
Oct 14, 2021

Conversation

skirui-source
Copy link
Contributor

Fixes: #6669

@skirui-source skirui-source added bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. cuIO cuIO issue labels Jul 14, 2021
@skirui-source skirui-source self-assigned this Jul 14, 2021
@skirui-source skirui-source changed the base branch from branch-21.08 to branch-21.10 July 27, 2021 23:45
cwharris
cwharris reviewed Aug 12, 2021
View reviewed changes
Copy link
Contributor

@cwharris cwharris left a comment

Choose a reason for hiding this comment

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

Overall looking good. I gave lots of feedback, but this is a good start. 👍

cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
@cwharris cwharris changed the title BIG FIX: CSV Writer ignores the header parameter when no metadata is provided BUG FIX: CSV Writer ignores the header parameter when no metadata is provided Aug 12, 2021
@skirui-source skirui-source requested a review from cwharris August 13, 2021 20:29
cwharris
cwharris reviewed Aug 13, 2021
View reviewed changes
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
@skirui-source skirui-source requested a review from cwharris August 31, 2021 23:42
@skirui-source skirui-source requested a review from vuule September 1, 2021 00:17
@skirui-source skirui-source marked this pull request as ready for review September 1, 2021 00:18
@skirui-source skirui-source requested a review from a team as a code owner September 1, 2021 00:18
@vuule vuule added the non-breaking Non-breaking change label Sep 1, 2021
vuule
vuule reviewed Sep 1, 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.

Silly question - how come the PR only includes a test?

cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
@skirui-source skirui-source added the 2 - In Progress Currently a work in progress label Sep 3, 2021
@skirui-source skirui-source marked this pull request as draft September 3, 2021 03:32
@skirui-source skirui-source requested a review from vuule September 24, 2021 23:54
vuule
vuule reviewed Sep 27, 2021
View reviewed changes
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
cpp/src/io/csv/writer_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/csv/writer_impl.cu Outdated Show resolved Hide resolved
vuule
vuule reviewed Sep 27, 2021
View reviewed changes
cpp/tests/io/csv_test.cpp Outdated Show resolved Hide resolved
@skirui-source skirui-source changed the base branch from branch-21.10 to branch-21.12 October 5, 2021 20:05
@skirui-source skirui-source marked this pull request as ready for review October 13, 2021 00:55
@codecov
Copy link

codecov bot commented Oct 13, 2021 •
edited
Loading

Codecov Report

Merging #8740 (80df448) into branch-21.12 (ab4bfaa) will increase coverage by 0.00%.
The diff coverage is 0.00%.

❗ Current head 80df448 differs from pull request most recent head 169dcdc. Consider uploading reports for the commit 169dcdc to get more accurate results
Impacted file tree graph

@@              Coverage Diff               @@
##           branch-21.12    #8740    +/-   ##
==============================================
  Coverage         10.79%   10.79%            
==============================================
  Files               116      116            
  Lines             18869    19443   +574     
==============================================
+ Hits               2036     2098    +62     
- Misses            16833    17345   +512
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/_lib/__init__.py 0.00% <ø> (ø)
python/cudf/cudf/_lib/strings/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/csv.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
... and 59 more

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 6dbea58...169dcdc. Read the comment docs.

vuule
vuule requested changes Oct 13, 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.

Few minor suggestions.
Thank you for tackling this issue!

cpp/src/io/csv/writer_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/csv/writer_impl.cu Outdated Show resolved Hide resolved
cpp/tests/io/csv_test.cpp Show resolved Hide resolved
@skirui-source
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 800fd7b into rapidsai:branch-21.12 Oct 14, 2021
@skirui-source skirui-source deleted the csv_writer branch October 19, 2021 20:06
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

@cwharris cwharris Awaiting requested review from cwharris

@kaatish kaatish Awaiting requested review from kaatish

Assignees

@skirui-source skirui-source

Labels
2 - In Progress Currently a work in progress bug Something isn't working cuIO cuIO issue 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.

[BUG] CSV Writer ignores the header parameter when no metadata is provided
4 participants
@skirui-source @cwharris @vuule @davidwendt