Add column_device_view to orc writer #7676

kaatish · 2021-03-23T04:51:13Z

This PR adds column_device_view members to EncChunk, DictionaryChunk and StripeDictionary structures which are used in the ORC writer. The idea is to replace members in these structures which replicate the same information. Usage of nvstrdesc_s has also been eliminated in the ORC writer.

Fixes #7347, Addresses #5682, Addresses #7334

Add column_device_view members

…rc-column-device-view

cpp/src/io/orc/writer_impl.cu

codecov · 2021-03-23T22:02:21Z

Codecov Report

Merging #7676 (d3a573e) into branch-0.19 (7871e7a) will increase coverage by 0.66%.
The diff coverage is n/a.

❗ Current head d3a573e differs from pull request most recent head 0a80942. Consider uploading reports for the commit 0a80942 to get more accurate results

@@               Coverage Diff               @@
##           branch-0.19    #7676      +/-   ##
===============================================
+ Coverage        81.86%   82.53%   +0.66%     
===============================================
  Files              101      101              
  Lines            16884    17453     +569     
===============================================
+ Hits             13822    14404     +582     
+ Misses            3062     3049      -13

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/decimal.py	`92.95% <0.00%> (-1.92%)`	⬇️
python/cudf/cudf/core/column/lists.py	`90.00% <0.00%> (-1.40%)`	⬇️
python/cudf/cudf/core/column/numerical.py	`94.83% <0.00%> (-0.20%)`	⬇️
python/cudf/cudf/core/column/column.py	`87.61% <0.00%> (-0.15%)`	⬇️
python/cudf/cudf/utils/utils.py	`85.36% <0.00%> (-0.07%)`	⬇️
python/cudf/cudf/io/feather.py	`100.00% <0.00%> (ø)`
python/cudf/cudf/utils/ioutils.py	`78.71% <0.00%> (ø)`
python/cudf/cudf/comm/serialize.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/_fuzz_testing/io.py	`0.00% <0.00%> (ø)`
python/cudf/cudf/core/column/struct.py	`100.00% <0.00%> (ø)`
... and 42 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 b271c06...0a80942. Read the comment docs.

…rc-column-device-view

vuule · 2021-03-24T04:58:35Z

Fixes #7347, #5682 Addresses #7334

You cannot apply "fixes" to multiple issues like this. Has to be "Fixes #7347, fixes #5682, addresses #7334"

vuule · 2021-03-24T05:32:44Z

Should this PR close #5682? I see uses of nvstrdesc_s that are not removed in this PR. I would expect for all nvstrdesc_s declaration to be removed before we close #5682.

vuule

Big fan of the changes. Just needs more span and to add missing docs. And the span use can be pushed to another PR IMO.

Have you run the benchmarks?

cpp/src/io/orc/writer_impl.hpp

cpp/src/io/orc/writer_impl.cu

vuule · 2021-03-24T05:55:52Z

cpp/src/io/orc/dict_enc.cu

+  gpuInitDictionaryIndices(DictionaryChunk *chunks,
+                           const table_device_view view,
+                           uint32_t *dict_data,
+                           uint32_t *dict_index,
+                           size_t row_index_stride,
+                           size_type *str_col_ids,
+                           uint32_t num_columns)


These should (almost) all be spans/2dspans, but we might want to leave this for another PR, given the urgency of this PR.

cpp/src/io/orc/dict_enc.cu

ttnghia · 2021-03-24T14:32:35Z

Fixes #7347, #5682 Addresses #7334

Please add more description. It's not obvious to know what this PR is doing unless we have to click/hover on those links to open these issues.

davidwendt · 2021-03-24T15:07:56Z

Fixes #7347, #5682 Addresses #7334

Please add more description. It's not obvious to know what this PR is doing unless we have to click/hover on those links to open these issues.

Also, the description gets added to the changelog entry so even a simple description here would be helpful.

kaatish · 2021-03-24T17:00:28Z

Have you run the benchmarks?

Performance impact is captured here.

kaatish · 2021-03-24T17:08:31Z

Should this PR close #5682? I see uses of nvstrdesc_s that are not removed in this PR. I would expect for all nvstrdesc_s declaration to be removed before we close #5682.

I have removed the link to that issue for now but it talks about removing nvstrdesc_s in the writers only which has been done in this PR.

vuule · 2021-03-24T17:29:06Z

Should this PR close #5682? I see uses of nvstrdesc_s that are not removed in this PR. I would expect for all nvstrdesc_s declaration to be removed before we close #5682.

I have removed the link to that issue for now but it talks about removing nvstrdesc_s in the writers only which has been done in this PR.

@devavret to clarify the issue. My interpretation is that all nvstrdesc_s uses should be replaced.

devavret · 2021-03-24T17:48:26Z

Should this PR close #5682? I see uses of nvstrdesc_s that are not removed in this PR. I would expect for all nvstrdesc_s declaration to be removed before we close #5682.

I have removed the link to that issue for now but it talks about removing nvstrdesc_s in the writers only which has been done in this PR.

@devavret to clarify the issue. My interpretation is that all nvstrdesc_s uses should be replaced.

That is true. It just might need one more PR. So it could be an "addresses" here.

cpp/src/io/orc/writer_impl.cu

cpp/src/io/orc/orc_gpu.h

vuule · 2021-03-24T23:21:07Z

rerun tests

vuule · 2021-03-25T05:14:08Z

@gpucibot merge

@kaatish

In PR #7676 the length of the current string being referred to while building stripe dictionaries was always set to 0 while incrementing the dictionary character count of a StripeDictionary. This led to corrupted strings when the dictionary encoding was used as noted in issue #7741. This has been fixed in this PR. Fixes #7741 Authors: - Kumar Aatish (@kaatish) Approvers: - Vukasin Milovanovic (@vuule) - Nghia Truong (@ttnghia) URL: #7744

kaatish added 4 commits March 17, 2021 13:29

Initial compilable commit

d954670

Reduce nvstrdesc_s usage

8afdbdf

Add column_device_view members

Style fix

2ec2e22

Merge branch 'branch-0.19' of https://github.com/rapidsai/cudf into o…

b788434

…rc-column-device-view

kaatish added 2 - In Progress Currently a work in progress code quality libcudf Affects libcudf (C++/CUDA) code. cuIO cuIO issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 23, 2021

kaatish requested review from vuule and rgsl888prabhu March 23, 2021 04:51

kaatish self-assigned this Mar 23, 2021

kaatish requested a review from a team as a code owner March 23, 2021 04:51

Style fix

f196683

kaatish commented Mar 23, 2021

View reviewed changes

cpp/src/io/orc/writer_impl.cu Outdated Show resolved Hide resolved

kaatish added 2 commits March 23, 2021 15:11

Fix init_dictionaries

9d0dee4

Add stream to table device view creation

17b8c36

kaatish added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Mar 23, 2021

Merge branch 'branch-0.19' of https://github.com/rapidsai/cudf into o…

6d8c1c8

…rc-column-device-view

vuule requested changes Mar 24, 2021

View reviewed changes

kaatish added 2 commits March 24, 2021 10:36

PR review fixes

ae31963

Remove nvstrdesc_s in writers and statistics

1b78c63

kaatish requested a review from vuule March 24, 2021 17:55

Add streams to hostdevice vectors

22cf657

vuule requested a review from devavret March 24, 2021 18:43

Pass string_view by value

7ae3587

vuule approved these changes Mar 24, 2021

View reviewed changes

devavret reviewed Mar 24, 2021

View reviewed changes

cpp/src/io/orc/writer_impl.cu Outdated Show resolved Hide resolved

devavret reviewed Mar 24, 2021

View reviewed changes

cpp/src/io/orc/orc_gpu.h Show resolved Hide resolved

devavret approved these changes Mar 24, 2021

View reviewed changes

PR review fixes

0a80942

rapids-bot bot merged commit f1f1d0f into rapidsai:branch-0.19 Mar 25, 2021

vuule mentioned this pull request Mar 26, 2021

[BUG] ORC string dictionary encoding corrupts some characters in the column #7741

Closed

kaatish mentioned this pull request Mar 26, 2021

Fix string length in stripe dictionary building #7744

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add column_device_view to orc writer #7676

Add column_device_view to orc writer #7676

kaatish commented Mar 23, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading

vuule commented Mar 24, 2021

vuule commented Mar 24, 2021

vuule left a comment

vuule Mar 24, 2021

ttnghia commented Mar 24, 2021

davidwendt commented Mar 24, 2021

kaatish commented Mar 24, 2021

kaatish commented Mar 24, 2021

vuule commented Mar 24, 2021

devavret commented Mar 24, 2021

vuule commented Mar 24, 2021

vuule commented Mar 25, 2021

Add column_device_view to orc writer #7676

Add column_device_view to orc writer #7676

Conversation

kaatish commented Mar 23, 2021 • edited Loading

codecov bot commented Mar 23, 2021 • edited Loading

Codecov Report

vuule commented Mar 24, 2021

vuule commented Mar 24, 2021

vuule left a comment

Choose a reason for hiding this comment

vuule Mar 24, 2021

Choose a reason for hiding this comment

ttnghia commented Mar 24, 2021

davidwendt commented Mar 24, 2021

kaatish commented Mar 24, 2021

kaatish commented Mar 24, 2021

vuule commented Mar 24, 2021

devavret commented Mar 24, 2021

vuule commented Mar 24, 2021

vuule commented Mar 25, 2021

kaatish commented Mar 23, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading