-
Notifications
You must be signed in to change notification settings - Fork 35
FEAT: Replace bulkcopy kwargs with explicit parameters #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?
Conversation
…IDE discoverability - All options now explicit in function signature - Pass params directly to pycore (no kwargs dict conversion) - Matches mssql-tds explicit params API Based on community feedback from discussion #414
c90f0f1 to
fa2eab4
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.
Pull request overview
Refactors Cursor._bulkcopy to replace **kwargs bulk-copy options with an explicit, type-hinted parameter list and updated docstring.
Changes:
- Replaced
**kwargsin_bulkcopywith explicit parameters (e.g.,batch_size,timeout,column_mappings, and bulk-copy flags). - Expanded
_bulkcopydocstring to document supported bulk copy options. - Updated the
pycore_cursor.bulkcopy(...)invocation to pass the new explicit parameters.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| column_mappings: Maps source data columns to target table column names. | ||
| Each tuple is (source, target_column_name) where: | ||
| - source: Column name (str) or 0-based index (int) in the source data |
Copilot
AI
Jan 30, 2026
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 doc/type for column_mappings now allows a string “source column name”, but the data contract above still says each row is a tuple/list (which has no column names). Please clarify the supported row types and mapping behavior, or restrict column_mappings back to index-based mapping to avoid misleading API/docs.
| - source: Column name (str) or 0-based index (int) in the source data | |
| - source: 0-based index (int) in the source data |
| result = pycore_cursor.bulkcopy( | ||
| table_name, | ||
| iter(data), | ||
| batch_size=batch_size, | ||
| timeout=timeout, | ||
| column_mappings=column_mappings, | ||
| keep_identity=keep_identity, | ||
| check_constraints=check_constraints, | ||
| table_lock=table_lock, | ||
| keep_nulls=keep_nulls, | ||
| fire_triggers=fire_triggers, | ||
| use_internal_transaction=use_internal_transaction, |
Copilot
AI
Jan 30, 2026
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 refactor changes behavior by always passing parameters like keep_identity, check_constraints, etc. to pycore_cursor.bulkcopy, even when they are None. Previously, omitted options would not be forwarded at all (via **kwargs), allowing the underlying library defaults to apply. To preserve semantics (and avoid potential TypeError if the core API expects bool), build a kwargs/options dict and only include values that are not None (or change defaults to explicit booleans).
| result = pycore_cursor.bulkcopy( | |
| table_name, | |
| iter(data), | |
| batch_size=batch_size, | |
| timeout=timeout, | |
| column_mappings=column_mappings, | |
| keep_identity=keep_identity, | |
| check_constraints=check_constraints, | |
| table_lock=table_lock, | |
| keep_nulls=keep_nulls, | |
| fire_triggers=fire_triggers, | |
| use_internal_transaction=use_internal_transaction, | |
| bulkcopy_kwargs = {} | |
| if batch_size is not None: | |
| bulkcopy_kwargs["batch_size"] = batch_size | |
| if timeout is not None: | |
| bulkcopy_kwargs["timeout"] = timeout | |
| if column_mappings is not None: | |
| bulkcopy_kwargs["column_mappings"] = column_mappings | |
| if keep_identity is not None: | |
| bulkcopy_kwargs["keep_identity"] = keep_identity | |
| if check_constraints is not None: | |
| bulkcopy_kwargs["check_constraints"] = check_constraints | |
| if table_lock is not None: | |
| bulkcopy_kwargs["table_lock"] = table_lock | |
| if keep_nulls is not None: | |
| bulkcopy_kwargs["keep_nulls"] = keep_nulls | |
| if fire_triggers is not None: | |
| bulkcopy_kwargs["fire_triggers"] = fire_triggers | |
| if use_internal_transaction is not None: | |
| bulkcopy_kwargs["use_internal_transaction"] = use_internal_transaction | |
| result = pycore_cursor.bulkcopy( | |
| table_name, | |
| iter(data), | |
| **bulkcopy_kwargs, |
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.
we're sending appropriate params, resolving this
| self, | ||
| table_name: str, | ||
| data: Iterable[Union[Tuple, List]], | ||
| batch_size: Optional[int] = None, |
Copilot
AI
Jan 30, 2026
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.
batch_size is typed as Optional[int], but the validation accepts floats (isinstance(batch_size, (int, float))) while the error message says “positive integer”. Please align the type hint, validation, and message (either require an int, or explicitly support non-integer values and document why).
| batch_size: Optional[int] = None, | |
| batch_size: Optional[Union[int, float]] = None, |
Work Item / Issue Reference
Summary
This pull request refactors the
_bulkcopymethod inmssql_python/cursor.pyto improve its interface and documentation. Instead of accepting arbitrary keyword arguments, the method now explicitly declares all supported bulk copy options as named parameters with type hints and default values.