Skip to content

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.

Sign up for GitHub

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

Open
bewithgaurav wants to merge 1 commit into
base: main
Choose a base branch
from
Open

FEAT: Replace bulkcopy kwargs with explicit parameters #420

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 49 additions & 17 deletions mssql_python/cursor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link

Copilot AI Jan 30, 2026

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).

Suggested change
batch_size: Optional[int] = None,
batch_size: Optional[Union[int, float]] = None,

Copilot uses AI. Check for mistakes.
timeout: Optional[int] = 30,
column_mappings: Optional[List[Tuple[Union[str, int], str]]] = None,
keep_identity: Optional[bool] = None,
check_constraints: Optional[bool] = None,
table_lock: Optional[bool] = None,
keep_nulls: Optional[bool] = None,
fire_triggers: Optional[bool] = None,
use_internal_transaction: Optional[bool] = None,
): # pragma: no cover
"""
Perform bulk copy operation for high-performance data loading.
Expand All @@ -2471,20 +2482,33 @@ def _bulkcopy(
- The number of values in each row must match the number of columns
in the target table

**kwargs: Additional bulk copy options.
batch_size: Number of rows to send per batch. Default uses server optimal.

timeout: Operation timeout in seconds. Default is 30.

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
Copy link

Copilot AI Jan 30, 2026

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.

Suggested change
- source: Column name (str) or 0-based index (int) in the source data
- source: 0-based index (int) in the source data

Copilot uses AI. Check for mistakes.
- target_column_name: Name of the target column in the database table

When omitted: Columns are mapped by ordinal position (first data
column → first table column, second → second, etc.)

When specified: Only the mapped columns are inserted; unmapped
source columns are ignored, and unmapped target columns must
have default values or allow NULL.

keep_identity: Preserve identity values from source data.

column_mappings (List[Tuple[int, str]], optional):
Maps source data column indices to target table column names.
Each tuple is (source_index, target_column_name) where:
- source_index: 0-based index of the column in the source data
- target_column_name: Name of the target column in the database table
check_constraints: Check constraints during bulk copy.

When omitted: Columns are mapped by ordinal position (first data
column → first table column, second → second, etc.)
table_lock: Use table-level lock instead of row-level locks.

When specified: Only the mapped columns are inserted; unmapped
source columns are ignored, and unmapped target columns must
have default values or allow NULL.
keep_nulls: Preserve null values instead of using default values.

fire_triggers: Fire insert triggers on the target table.

use_internal_transaction: Use an internal transaction for each batch.

Returns:
Dictionary with bulk copy results including:
Expand Down Expand Up @@ -2523,10 +2547,6 @@ def _bulkcopy(
f"data must be an iterable of tuples or lists, got non-iterable {type(data).__name__}"
)

# Extract and validate kwargs with defaults
batch_size = kwargs.get("batch_size", None)
timeout = kwargs.get("timeout", 30)

# Validate batch_size type and value (only if explicitly provided)
if batch_size is not None:
if not isinstance(batch_size, (int, float)):
Expand Down Expand Up @@ -2599,7 +2619,19 @@ def _bulkcopy(
pycore_connection = mssql_py_core.PyCoreConnection(pycore_context)
pycore_cursor = pycore_connection.cursor()

result = pycore_cursor.bulkcopy(table_name, iter(data), **kwargs)
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,
Comment on lines +2622 to +2633
Copy link

Copilot AI Jan 30, 2026

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).

Suggested change
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,

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@bewithgaurav bewithgaurav Jan 30, 2026

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

)

return result

Expand Down
Loading