FIX: FetchMany(number of rows) ignores batch size with LOB columns#346
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where FetchMany(number of rows) ignores the batch size parameter when tables contain LOB (Large Object) columns such as NVARCHAR(MAX). The fix ensures that when LOB columns are detected, the row-by-row fetch path properly limits the number of rows fetched to the specified batch size.
Key Changes:
- Modified the LOB fetch loop in C++ to respect the
fetchSizeparameter by adding a loop condition check - Added a new
lob_wvarchar_column(NVARCHAR(MAX)) column to the test table schema to enable LOB testing - Created dedicated test functions to validate fetch operations (fetchone, fetchmany, fetchall) with LOB columns present
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| mssql_python/pybind/ddbc_bindings.cpp | Fixed FetchMany_wrap to break the LOB fetch loop when numRowsFetched reaches fetchSize, preventing infinite fetching |
| tests/test_004_cursor.py | Added lob_wvarchar_column to test table, updated test data, and added new test functions to validate fetch behavior with LOB columns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dlevy-msft-sql
changed the title
dlevy-msft-sql
changed the title
dlevy-msft-sql
changed the title
dlevy-msft-sql
added
bug
sumitmsft
left a comment
sumitmsft
left a comment
There was a problem hiding this comment.
Left a few comments to be addressed.
@gargsaumya please review this PR.
|
@sumitmsft this one is ready for review again. The code coverage report issues seem to be breaking the workflow. |
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 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.helpers.py: 67.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.4%
mssql_python.pybind.ddbc_bindings.h: 71.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: 83.9%🔗 Quick Links
|
There was a problem hiding this comment.
LGTM!
This PR will also correctly resolve #390, validated this locally with VARCHAR(MAX) columns. Approving.
gargsaumya
dismissed
sumitmsft’s stale review
Closing this review - the fix has been validated.
4 participants
Work Item / Issue Reference
Summary
Fixes FetchMany(number of rows) ignores batch size when table contains an LOB