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

[REVIEW] Remove string_view is_null method #4548

Merged
merged 17 commits into from
Mar 23, 2020

Conversation

davidwendt
Copy link
Contributor

Closes #3346
The string_view class has three states: non-empty, empty, and null.
The null-state should not be required since we have a string_scalar class now.
Places where the string_view:is_null() function are being used can be reworked.
These were only found in strings::fill(), strings::concatenate() and strings:join_strings() which all accept string_scalar parameters.

Removing this method also means that string_view has only 2 states now: empty or non-empty. This should solve the requirement of all-empty strings column requiring a 1-byte chars column. This column was needed to distinguish between null and empty when creating a string_view in methods like string_scalar::value() and column_device_view::element<string_view>(). Since this is no longer possible, special logic is not required for handling a null string_view instance.
Therefore, any workaround logic in string.py and strings/utilities.cu to preserve the 1-byte chars column for all-empty strings column is no longer required.

@davidwendt davidwendt self-assigned this Mar 17, 2020
@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. labels Mar 17, 2020
@codecov
Copy link

codecov bot commented Mar 17, 2020

Codecov Report

Merging #4548 into branch-0.14 will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.14    #4548      +/-   ##
===============================================
- Coverage        88.42%   88.42%   -0.01%     
===============================================
  Files               50       50              
  Lines             9686     9681       -5     
===============================================
- Hits              8565     8560       -5     
  Misses            1121     1121              
Impacted Files Coverage Δ
python/cudf/cudf/core/column/string.py 86.11% <ø> (-0.10%) ⬇️

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 234eef0...81df486. Read the comment docs.

@davidwendt davidwendt marked this pull request as ready for review March 21, 2020 00:43
@davidwendt davidwendt requested review from a team as code owners March 21, 2020 00:43
@davidwendt davidwendt changed the title [WIP] Remove string_view is_null method [REVIEW] Remove string_view is_null method Mar 21, 2020
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Mar 21, 2020
Copy link
Contributor

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

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

Minor changes, other than. LGTM

cpp/src/strings/utilities.cu Outdated Show resolved Hide resolved
cpp/src/strings/utilities.cu Outdated Show resolved Hide resolved
@davidwendt davidwendt added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Mar 23, 2020
@davidwendt davidwendt merged commit 93cb978 into rapidsai:branch-0.14 Mar 23, 2020
@davidwendt davidwendt deleted the remove-string-view-is-null branch March 23, 2020 23:24
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 libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants