-
Notifications
You must be signed in to change notification settings - Fork 919
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 Python bindings for string literal support in AST #13073
Merged
rapids-bot
merged 17 commits into
rapidsai:branch-23.06
from
karthikeyann:fea-cython-string_scalar_ast_compare
May 8, 2023
Merged
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
982af8a
string scalar support in AST - proof of concept
karthikeyann 0a9eb86
Add cudf::ast::generic_scalar_device_view
karthikeyann 50ee55d
remove filter by range example from test code
karthikeyann 9735d51
cleanup docs
karthikeyann 8653e61
Merge branch 'branch-23.06' of github.com:rapidsai/cudf into fea-stri…
karthikeyann 7ad5c5d
add cython bindings, unit tests for string literal support in AST
karthikeyann 3a40c31
Apply suggestions from code review
karthikeyann 24b6589
Merge branch 'branch-23.06' into fea-cython-string_scalar_ast_compare
karthikeyann 3405241
Merge branch 'branch-23.06' into fea-cython-string_scalar_ast_compare
karthikeyann 4c44afb
cleanup cython Literal, update docs
karthikeyann 74cd710
Merge branch 'branch-23.06' of github.com:rapidsai/cudf into fea-cyth…
karthikeyann 5d80737
Merge branch 'branch-23.06' into fea-cython-string_scalar_ast_compare
karthikeyann c3d8b66
Merge branch 'branch-23.06' into fea-cython-string_scalar_ast_compare
karthikeyann 215d4db
Merge branch 'branch-23.06' into fea-cython-string_scalar_ast_compare
karthikeyann fa31360
Update python/cudf/cudf/core/dataframe.py
vyasr ab9fd3d
Merge branch 'branch-23.06' into fea-cython-string_scalar_ast_compare
vyasr 30c4b96
Merge branch 'branch-23.06' into fea-cython-string_scalar_ast_compare
karthikeyann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
new
have a correspondingdelete
? Note that if we need to calldelete
, it can be done in the__dealloc__
method of this class__dealloc__
or callingdelete
?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.
Oh I think I see what's going here, we're resetting the value of the unique pointer to a new
numeric_scalar
. But why do it this way instead ofmake_unique
?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.
c_scalar
is a unique pointer here. This line is calling unique_ptr constructor where argument is the object pointer. This unique pointer will delete the pointing object when it's destroyed.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.
make_unique
creates thenumeric_scalar
unique pointer. It can't be type casted and stored to generic base pointer. Hence this approach. I saw similar approach in another Cython file as well.https://github.com/rapidsai/cudf/blob/branch-23.06/python/cudf/cudf/_lib/scalar.pyx#L250
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.
Thanks! I approved the PR, but still have a question. Why prefer:
versus:
?
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.
It would require releasing the unique_ptr of derived pointer anyway to store in
unique_ptr<scalar>
class.unique_ptr[scalar] c_scalar = unique_ptr(static_cast<scalar*>(make_unique(...).release()));
Hence using the new operator directly.
Is 'releaseing the unique_ptr and typecasting' safer than using
reset(new...
?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.
Got it - thanks for clarifying! Definitely a TIL for me
Do you mean is it safer from a Cython perspective? If so, the answer is that it should be no different than in C++. I would say whatever pattern you prefer in C++, we should use here as well.