-
Notifications
You must be signed in to change notification settings - Fork 36
FEAT: Explicit parameters and improved type hints for bulkcopy #420
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
fa2eab4
03714c7
f01756b
f1dc84a
f6824a3
a199422
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2452,7 +2452,18 @@ def nextset(self) -> Union[bool, None]: | |||||
| return True | ||||||
|
|
||||||
| def _bulkcopy( | ||||||
| self, table_name: str, data: Iterable[Union[Tuple, List]], **kwargs | ||||||
| self, | ||||||
| table_name: str, | ||||||
| data: Iterable[Union[Tuple, List]], | ||||||
| batch_size: Optional[int] = None, | ||||||
|
||||||
| batch_size: Optional[int] = None, | |
| batch_size: Optional[Union[int, float]] = None, |
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.
Could you please provide more details in the PR summary for this change?
This API change may not be scalable in future as any new parameter addition need to go through the API contract change which is not advisable.
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 conversation around new API spec is happening in Github Discussions topic #414
I'll update the PR summary with the new spec details and link to the above discussion shortly
will ping once done
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.
done - updated, pls recheck and let me know if you have any comments
Uh oh!
There was an error while loading. Please reload this page.
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 am still not convinced about moving out from **kwargs completly.
Can we group the parameters logically with couple of parameter?
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.
Hi @subrata-ms, can you please be more specific with regard to why you're not convinced here? We discussed in the thread linked above and those of us on the end user side who decided to participate seemed to agree that this is a good design. As someone who has worked with SQL Server bulk copy libraries for over 20 years in a number of different languages I feel that this is the best possible option from a usability perspective: The parameters will be obvious and easily discovered by users. Introducing a required external object here won't be especially helpful and users will just have to jump through another hoop when making small changes. And while it does seem like there are a touch more parameters here than we usually see, the real concern should be forward maintainability -- but there is none. These basic parameters haven't changed in all of the time I've been working with SQL Server and I seriously doubt they ever will.
Uh oh!
There was an error while loading. Please reload this page.
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.
Hi @amachanic , I would suggest, we discuss these API design approach options in the API Design Review forum so we can gather broader feedback. ( #414 ).