TEST: _bulkcopy kwargs passing to mssql_py_core #413
Conversation
The Rust API expects kwargs as a dict parameter (kwargs=dict), not as Python keyword arguments (**kwargs). Before: pycore_cursor.bulkcopy(table_name, iter(data), **kwargs) After: pycore_cursor.bulkcopy(table_name, iter(data), kwargs=kwargs)
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 69.4%
mssql_python.pybind.ddbc_bindings.h: 69.7%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 84.1%
mssql_python.cursor.py: 84.7%🔗 Quick Links
|
bewithgaurav
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in how keyword arguments are passed to the mssql_py_core Rust API's bulkcopy method. The previous implementation incorrectly used Python's **kwargs unpacking syntax, but the Rust API expects kwargs as a named dictionary parameter.
Changes:
- Updated the
pycore_cursor.bulkcopy()call to passkwargsas a named parameter (kwargs=kwargs) instead of unpacking with**kwargs - Added clarifying comments explaining the Rust API's expected signature
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mssql_python/cursor.py
Outdated
| # Pass kwargs as a dict parameter, not as Python keyword arguments | ||
| # The Rust API expects: bulkcopy(table_name, data_source, kwargs=dict) | ||
| result = pycore_cursor.bulkcopy( | ||
| table_name, iter(data), kwargs=kwargs if kwargs else None |
There was a problem hiding this comment.
The conditional expression kwargs if kwargs else None will pass None when kwargs is an empty dict, since empty dicts are falsy in Python. This means when _bulkcopy is called without any kwargs, None will be passed to the Rust API instead of an empty dict. Consider whether the Rust API expects None or an empty dict when no options are provided. If an empty dict should always be passed, change this to just kwargs=kwargs.
| table_name, iter(data), kwargs=kwargs if kwargs else None | |
| table_name, iter(data), kwargs=kwargs |
The Rust API now accepts **kwargs directly, so we can use the cleaner Python syntax.
…/microsoft/mssql-python into bewithgaurav/fix-bulkcopy-kwargs
bewithgaurav
changed the title
|
closing the test PR |
Work Item / Issue Reference
Summary
The mssql-py-core API expects kwargs as a dict parameter (kwargs=dict), not as Python keyword arguments (**kwargs).
Before: pycore_cursor.bulkcopy(table_name, iter(data), **kwargs)
After: pycore_cursor.bulkcopy(table_name, iter(data), kwargs=kwargs)
This pull request updates the way keyword arguments are passed to the
bulkcopymethod in themssql_python/cursor.pyfile. The main change ensures compatibility with the Rust API by passingkwargsas a dictionary parameter rather than as Python keyword arguments.pycore_cursor.bulkcopyto passkwargsas a dictionary using thekwargsparameter, aligning with the expectations of the Rust API.