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

Remove nvstrdesc_s from cuio #7841

Merged
merged 8 commits into from
Apr 17, 2021
Merged

Remove nvstrdesc_s from cuio #7841

merged 8 commits into from
Apr 17, 2021

Conversation

kaatish
Copy link
Contributor

@kaatish kaatish commented Apr 2, 2021

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.

@kaatish 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 kaatish self-assigned this Apr 2, 2021
@kaatish kaatish requested review from a team as code owners April 2, 2021 14:30
@kaatish kaatish requested review from cwharris and nvdbaranec April 2, 2021 14:30
@github-actions github-actions bot added conda Java Affects Java cuDF API. labels Apr 2, 2021
@kaatish kaatish requested review from vuule and devavret and removed request for cwharris April 2, 2021 14:31
@kaatish kaatish changed the base branch from branch-0.19 to branch-0.20 April 2, 2021 14:32
@github-actions github-actions bot removed Java Affects Java cuDF API. conda labels Apr 2, 2021
@kaatish kaatish added the 3 - Ready for Review Ready for review by team label Apr 2, 2021
@vuule
Copy link
Contributor

vuule commented Apr 2, 2021

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

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 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 Outdated Show resolved Hide resolved
cpp/src/io/orc/stripe_data.cu Outdated Show resolved Hide resolved
cpp/src/io/avro/avro_common.h Outdated Show resolved Hide resolved
cpp/src/io/parquet/parquet_gpu.hpp Outdated Show resolved Hide resolved
cpp/src/io/utilities/column_buffer.hpp Outdated Show resolved Hide resolved
@vuule
Copy link
Contributor

vuule commented Apr 2, 2021

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

@kaatish kaatish requested a review from vuule April 4, 2021 01:19
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.

👍

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.

🥳

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 raydouglass removed the request for review from a team April 5, 2021 14:40
@raydouglass
Copy link
Member

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.

Copy link
Contributor

@nvdbaranec nvdbaranec 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 to me. I think Devavret's comment on the include style might need to be addressed though.

cpp/src/io/utilities/column_buffer.hpp Show resolved Hide resolved
@kaatish kaatish requested a review from harrism April 6, 2021 15:23
@jrhemstad
Copy link
Contributor

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

@kkraus14 kkraus14 removed the request for review from a team April 6, 2021 21:40
@vuule
Copy link
Contributor

vuule commented Apr 6, 2021

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
Copy link
Contributor

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
Copy link
Contributor

devavret commented Apr 7, 2021

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.

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.

@kaatish kaatish requested a review from devavret April 16, 2021 21:01
@codecov
Copy link

codecov bot commented Apr 16, 2021

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
Impacted file tree graph

@@               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.

Copy link
Collaborator

@kkraus14 kkraus14 left a comment

Choose a reason for hiding this comment

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

CMake approval

@devavret
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1d03186 into rapidsai:branch-0.20 Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CMake build issue cuIO cuIO issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Replace nvstrdesc with string_view
8 participants