Remove nvstrdesc_s from cuio #7841

kaatish · 2021-04-02T14:30:37Z

Fixes #5682.

Structure nvstrdesc_s was replaced with thrust::pair<const char*, size_type>;.
nvstrdesc_s related logical functions such as nvstr_is_lesser, nvstr_is_greater etc. were removed.
Include directives for headers included by source files residing in the same directory were made relative as per the developer guide.
make_column function related to column_buffer was moved from a header file to an implementation file.

vuule · 2021-04-02T22:03:26Z

Can you please add a high-level list of changes to the description, and list the issue(s) this PR addresses?

vuule

Looks great!
A couple of small asks, just to be as consistent as possible with the rest of libcudf.

cpp/src/io/utilities/column_buffer.hpp

cpp/src/io/orc/stripe_data.cu

cpp/src/io/avro/avro_common.h

cpp/src/io/parquet/parquet_gpu.hpp

cpp/src/io/utilities/column_buffer.hpp

vuule · 2021-04-02T22:43:02Z

@kkraus14 this PR has been incorrectly marked to require Java and Ops reviews. Was there a way to clear this?

vuule

👍

devavret

🥳

cpp/src/io/comp/brotli_dict.cpp

cpp/src/io/json/json.h

cpp/src/io/utilities/column_buffer.cpp

raydouglass · 2021-04-05T14:40:50Z

Removing ops-codeowners from the required reviews since it doesn't seem there are any file changes that we're responsible for. Feel free to add us back if necessary.

nvdbaranec

Looks good to me. I think Devavret's comment on the include style might need to be addressed though.

cpp/src/io/utilities/column_buffer.hpp

jrhemstad · 2021-04-06T21:22:57Z

Why not use string_view instead of thrust::pair<const char*, size_type>?

vuule · 2021-04-06T21:57:04Z

Why not use string_view instead of thrust::pair<const char*, size_type>?

string_view is larger than the pair because of the size related data members. We really want to avoid memory use increases in these APIs.

nvdbaranec · 2021-04-06T22:16:15Z

Why not use string_view instead of thrust::pair<const char*, size_type>?

string_view is larger than the pair because of the size related data members. We really want to avoid memory use increases in these APIs.

I don't think that's the case. The whole struct is going to have to be aligned to 8 bytes because of the pointer anyway.

devavret · 2021-04-07T14:56:52Z

Why not use string_view instead of thrust::pair<const char*, size_type>?

string_view is larger than the pair because of the size related data members. We really want to avoid memory use increases in these APIs.

I don't think that's the case. The whole struct is going to have to be aligned to 8 bytes because of the pointer anyway.

string_view contains 4 members.

cudf/cpp/include/cudf/strings/string_view.hpp

Lines 337 to 340 in ab7fe05

    
           const char* _data{};           ///< Pointer to device memory contain char array for this string 
        
           size_type _bytes{};            ///< Number of bytes in _data for this string 
        
           mutable size_type _length{};   ///< Number of characters in this string (computed) 
        
           mutable int8_t _char_width{};  ///< Number of bytes per character if uniform width (computed)

2 of these are calculated using the other 2. This is what @vuule is referring to as size related members.

BTW, @jrhemstad, the memory requirement is in column_buffer which is going to allocate a vector of these <data, size> pairs.

…e-nvstrdesc

codecov · 2021-04-16T23:07:17Z

Codecov Report

Merging #7841 (efdec2b) into branch-0.20 (51336df) will decrease coverage by 0.37%.
The diff coverage is 87.64%.

❗ Current head efdec2b differs from pull request most recent head 7e5efd7. Consider uploading reports for the commit 7e5efd7 to get more accurate results

@@               Coverage Diff               @@
##           branch-0.20    #7841      +/-   ##
===============================================
- Coverage        82.88%   82.51%   -0.38%     
===============================================
  Files              103      103              
  Lines            17668    17296     -372     
===============================================
- Hits             14645    14272     -373     
- Misses            3023     3024       +1

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/__init__.py	`100.00% <ø> (ø)`
python/cudf/cudf/utils/cudautils.py	`55.04% <25.00%> (-2.72%)`	⬇️
python/cudf/cudf/core/column/numerical.py	`94.41% <70.00%> (-0.02%)`	⬇️
python/cudf/cudf/core/column/column.py	`88.48% <71.42%> (-0.17%)`	⬇️
python/cudf/cudf/core/dataframe.py	`90.75% <83.33%> (-0.11%)`	⬇️
python/cudf/cudf/utils/utils.py	`89.47% <91.66%> (-0.04%)`	⬇️
python/cudf/cudf/core/column/datetime.py	`89.40% <100.00%> (-0.52%)`	⬇️
python/cudf/cudf/core/column/timedelta.py	`88.33% <100.00%> (-0.34%)`	⬇️
python/cudf/cudf/core/groupby/groupby.py	`92.12% <100.00%> (+0.67%)`	⬆️
python/cudf/cudf/core/index.py	`92.62% <100.00%> (-0.46%)`	⬇️
... and 53 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 98711da...7e5efd7. Read the comment docs.

kkraus14

CMake approval

devavret · 2021-04-17T00:46:41Z

@gpucibot merge

Remove nvstrdesc_s

3791668

kaatish added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue cuIO cuIO issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 2, 2021

kaatish self-assigned this Apr 2, 2021

kaatish requested review from a team as code owners April 2, 2021 14:30

kaatish requested review from cwharris and nvdbaranec April 2, 2021 14:30

github-actions bot added conda Java Affects Java cuDF API. labels Apr 2, 2021

kaatish requested review from vuule and devavret and removed request for cwharris April 2, 2021 14:31

kaatish changed the base branch from branch-0.19 to branch-0.20 April 2, 2021 14:32

Replace relative paths with absolute paths

197bf73

github-actions bot removed Java Affects Java cuDF API. conda labels Apr 2, 2021

Style fix

5ccbf3f

kaatish added the 3 - Ready for Review Ready for review by team label Apr 2, 2021

vuule requested changes Apr 2, 2021

View reviewed changes

PR review fixes

f0a5b47

Fix json

4276c3a

kaatish requested a review from vuule April 4, 2021 01:19

vuule approved these changes Apr 4, 2021

View reviewed changes

devavret suggested changes Apr 5, 2021

View reviewed changes

cpp/src/io/comp/brotli_dict.cpp Outdated Show resolved Hide resolved

cpp/src/io/json/json.h Outdated Show resolved Hide resolved

cpp/src/io/utilities/column_buffer.cpp Outdated Show resolved Hide resolved

raydouglass removed the request for review from a team April 5, 2021 14:40

nvdbaranec approved these changes Apr 6, 2021

View reviewed changes

cpp/src/io/utilities/column_buffer.hpp Show resolved Hide resolved

kaatish requested a review from harrism April 6, 2021 15:23

kkraus14 removed the request for review from a team April 6, 2021 21:40

kaatish added 3 commits April 7, 2021 13:42

PR review fixes

6e79922

Merge branch 'branch-0.20' of github.com:rapidsai/cudf into orc-remov…

218c6d9

…e-nvstrdesc

PR review fix

7e5efd7

kaatish requested a review from devavret April 16, 2021 21:01

devavret approved these changes Apr 16, 2021

View reviewed changes

kkraus14 approved these changes Apr 17, 2021

View reviewed changes

rapids-bot bot merged commit 1d03186 into rapidsai:branch-0.20 Apr 17, 2021

vyasr mentioned this pull request May 29, 2024

[BUG] A data buffer is cast into a type with a different layout #5916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove nvstrdesc_s from cuio #7841

Remove nvstrdesc_s from cuio #7841

kaatish commented Apr 2, 2021 •

edited

Loading

vuule commented Apr 2, 2021

vuule left a comment

vuule commented Apr 2, 2021

vuule left a comment

devavret left a comment

raydouglass commented Apr 5, 2021

nvdbaranec left a comment

jrhemstad commented Apr 6, 2021

vuule commented Apr 6, 2021

nvdbaranec commented Apr 6, 2021

devavret commented Apr 7, 2021

codecov bot commented Apr 16, 2021

kkraus14 left a comment

devavret commented Apr 17, 2021

Remove nvstrdesc_s from cuio #7841

Remove nvstrdesc_s from cuio #7841

Conversation

kaatish commented Apr 2, 2021 • edited Loading

vuule commented Apr 2, 2021

vuule left a comment

Choose a reason for hiding this comment

vuule commented Apr 2, 2021

vuule left a comment

Choose a reason for hiding this comment

devavret left a comment

Choose a reason for hiding this comment

raydouglass commented Apr 5, 2021

nvdbaranec left a comment

Choose a reason for hiding this comment

jrhemstad commented Apr 6, 2021

vuule commented Apr 6, 2021

nvdbaranec commented Apr 6, 2021

devavret commented Apr 7, 2021

codecov bot commented Apr 16, 2021

Codecov Report

kkraus14 left a comment

Choose a reason for hiding this comment

devavret commented Apr 17, 2021

kaatish commented Apr 2, 2021 •

edited

Loading