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

[REVIEW] Port NVStrings regex contains ops #3292

Merged
merged 58 commits into from
Jan 9, 2020
Merged

[REVIEW] Port NVStrings regex contains ops #3292

merged 58 commits into from
Jan 9, 2020

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Nov 4, 2019 •
edited
Loading

This will port NVStrings contains, match, and count operations that use regex to cudf strings column. This will be the first regex enabled strings column.

This PR depends on #3159 for the is-char-types flags table.

@davidwendt davidwendt requested review from a team as code owners November 4, 2019 20:50
@davidwendt davidwendt self-assigned this Nov 4, 2019
@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf++ strings strings issues (C++ and Python) labels Nov 4, 2019
@codecov
Copy link

codecov bot commented Nov 4, 2019 •
edited
Loading

Codecov Report

Merging #3292 into branch-0.12 will decrease coverage by 0.5%.
The diff coverage is 93.48%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.12    #3292      +/-   ##
===============================================
- Coverage        87.43%   86.92%   -0.51%     
===============================================
  Files               49       50       +1     
  Lines             9446     9346     -100     
===============================================
- Hits              8259     8124     -135     
- Misses            1187     1222      +35
Impacted Files Coverage Δ
python/cudf/cudf/core/column/__init__.py 100% <ø> (ø) ⬆️
python/cudf/cudf/_libxx/__init__.py 100% <100%> (ø)
python/cudf/cudf/core/table.py 100% <100%> (ø)
python/cudf/cudf/utils/cudautils.py 44.66% <100%> (-3.39%) ⬇️
python/cudf/cudf/utils/queryutils.py 94.05% <100%> (ø) ⬆️
python/cudf/cudf/core/reshape.py 88.14% <100%> (ø) ⬆️
python/cudf/cudf/utils/applyutils.py 100% <100%> (ø) ⬆️
python/dask_cudf/dask_cudf/backends.py 94.23% <100%> (-0.32%) ⬇️
python/cudf/cudf/core/groupby/legacy_groupby.py 77.63% <100%> (ø) ⬆️
python/cudf/cudf/core/_sort.py 90.62% <100%> (-0.56%) ⬇️
... and 28 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 2b07837...4e295fe. Read the comment docs.

codereport
codereport suggested changes Dec 11, 2019
View reviewed changes
Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

Posting partial review for now, will follow up tomorrow.

cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regcomp.cpp Show resolved Hide resolved
codereport
codereport suggested changes Dec 12, 2019
View reviewed changes
Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

Please review comments. The less trivial comments / changes I have omitted and put in the follow up issue: #3582

cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regcomp.cpp Outdated Show resolved Hide resolved
cpp/src/strings/regex/regexec.cu Outdated Show resolved Hide resolved
cpp/src/strings/regex/regex.inl Outdated Show resolved Hide resolved
cpp/src/strings/regex/regex.inl Outdated Show resolved Hide resolved
cpp/src/strings/regex/regex.inl Outdated Show resolved Hide resolved
cpp/src/strings/regex/regex.inl Outdated Show resolved Hide resolved
@codereport codereport added the 0 - Waiting on Author Waiting for author to respond to review label Dec 13, 2019
@davidwendt davidwendt removed the 0 - Waiting on Author Waiting for author to respond to review label Dec 17, 2019
@codereport codereport mentioned this pull request Dec 17, 2019
Closed
codereport
codereport approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@codereport codereport 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!

@davidwendt davidwendt mentioned this pull request Dec 18, 2019
Closed
12 tasks
@abellina abellina mentioned this pull request Jan 3, 2020
Closed
@jrhemstad jrhemstad merged commit 42218bb into rapidsai:branch-0.12 Jan 9, 2020
@davidwendt davidwendt deleted the port-nvs-regex-contains branch January 13, 2020 14:16
@davidwendt davidwendt mentioned this pull request Jan 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harrism harrism harrism left review comments

@codereport codereport codereport approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team strings strings issues (C++ and Python)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidwendt @harrism @jrhemstad @codereport