-
Notifications
You must be signed in to change notification settings - Fork 23
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 Support for Parameter Binding Arrays #47
Conversation
I've taken a first look at this. It looks quite good, though I've made some small improvements. I plan to take another look tomorrow. |
@godlygeek sounds good. Looks like DCO is failing |
Yep, that's fine. I didn't bother signing my fixup commits, since I plan to squash them into your commits. |
This PR now looks good to me, though I'd like another set of eyes on it (probably @gusmonod) since I made some changes here myself. We're going to hold off on merging this PR for a little while, though. We're gonna merge this after we cut a new release that officially drops support for versions prior to Python 3.7 (hopefully later this week). |
Sounds good, thanks for the review. Looking forward to getting it merged! 😄 |
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.
There is one thing that doesn't make sense to me, and that is the allocation of the memory for the arrays of bytes.
I wonder if maybe it needs to be done because of the fact that the data in python list
s and tuple
s may not be contiguous… 🤔 either way, would like to know what you all think about it.
f951026
to
106bbe2
Compare
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.
LGTM. I've just rebased it to check that the tests do indeed all pass, and I have changed six.integer_type
to int
instead in the test code (now that we no longer use six
).
The only thing missing for this to be merged is the merging of all commits, @athmihir would you please do it?
50f7a60
to
3dcd0ef
Compare
Signed-off-by: asardesai2 <[email protected]>
3dcd0ef
to
203f0f2
Compare
Issue number of the reported bug or feature request: #
Describe your changes
Add the ability to parameter bind carrays in dbapi2 through python lists, or tuples of type: int, float. string or bytes.
This is by exposing the cdb2_bind_array function present in cdb2api.
Testing performed
Add pytests that try all combinations of accepted parameters. ie. lists and tuples of allowed data types.
Additional context
None.