Add Python bindings for string literal support in AST #13073
Conversation
…ng_scalar_ast_compare
karthikeyann
added
feature request
karthikeyann
requested review from
shwina,
skirui-source,
PointKernel and
nvdbaranec
Co-authored-by: Vyas Ramasubramani <[email protected]>
|
Removing libcudf review requests since it's no longer relevant |
| self.c_scalar.int_ptr = make_unique[numeric_scalar[int64_t]]( | ||
| intval, True | ||
| ) | ||
| self.c_scalar.reset(new numeric_scalar[int64_t](value, True)) |
There was a problem hiding this comment.
- Shouldn't every
newhave a correspondingdelete? Note that if we need to calldelete, it can be done in the__dealloc__method of this class - Can we continue to use a unique/shared pointer so we don't have to worry about defining
__dealloc__or callingdelete?
There was a problem hiding this comment.
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 of make_unique?
There was a problem hiding this comment.
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.
make_unique creates the numeric_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.
Thanks! I approved the PR, but still have a question. Why prefer:
c_scalar.reset(new ....)
versus:
c_scalar = make_unique(...)
?
There was a problem hiding this comment.
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.
Got it - thanks for clarifying! Definitely a TIL for me
Is 'releaseing the unique_ptr and typecasting' safer than using reset(new... ?
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.
|
/merge |
Depends on rapidsai#13061 Add Python bindings for string scalar support in AST Add unit test for string comparison - column vs column, column vs literal. Authors: - Karthikeyan (https://github.com/karthikeyann) - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Ashwin Srinath (https://github.com/shwina) URL: rapidsai#13073
vyasr
added
4 - Needs Review
Description
Depends on #13061
Add Python bindings for string scalar support in AST
Add unit test for string comparison - column vs column, column vs literal.
Checklist