Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add cudf::strings::find_re API #16742
Add cudf::strings::find_re API #16742
Changes from 25 commits
664edcb
6312aaa
daa106e
96933c9
c326928
f544e6c
108aca6
bab22d7
99651c1
f703265
3caea9d
3fdcfd2
8762aed
bd08864
5041c42
04f3bde
d1065f5
a770475
8d9dc71
1286abc
2305d43
1afe3ca
ffd2893
2a913ca
01dc722
e759005
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this go in a new file, or should this file be renamed? It seems incorrect to have
find_re
defined in thefindall
file, since they're different algorithms.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was told to keep match these with the header files. So
find_re
andfindall
are both declared infindall.hpp
. They are common only in that both use regex I suppose.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize
find_re
was infindall.hpp
. Then yes, this is fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(But it does raise the question of why
find_re
is infindall.hpp
-- it doesn't "find all")There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC they will be combined into a common file as per https://github.com/rapidsai/cudf/pull/16742/files#r1781466464
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
find_re
has more in common withfindall
than justfind
based on the parameters and how they are used just in not what they return.Perhaps
find_re()
would be better incontains.hpp
which hascontains_re()
Since
find.hpp
hascontains()
andfind()
(among other similar functions)So
contains.hpp
would havecontains_re()
andfind_re()
.I could do that in a separate PR since this one is already approved.