FIX: Preserve leading BOM-like characters in bounded text fetches - #793
Open
Jahnvi Thakkar (jahnvi480) wants to merge 2 commits into
Open
Jahnvi Thakkar (jahnvi480) wants to merge 2 commits into
Jahnvi Thakkar (jahnvi480) wants to merge 2 commits into
Conversation
Decode bounded ODBC wide-text payloads strictly in native byte order without consuming leading payload characters. Add regression coverage and document the intentional correctness change and unchanged MAX limitations. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 17, 2026 12:29
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The native-endian selection has a correctness defect on big-endian builds, and cross-platform validation remains pending.
Pull request overview
Preserves leading BOM-like characters in bounded text fetches through strict native-endian decoding.
Changes:
- Added shared UTF-16 decoding helper.
- Updated bounded text conversion paths.
- Added regression tests and changelog documentation.
File summaries
| File | Summary |
|---|---|
tests/test_017_fetch_bounded_text.py |
Adds bounded text regression coverage. |
mssql_python/pybind/fetch_text.hpp |
Adds strict UTF-16 decoding helper. |
mssql_python/pybind/ddbc_bindings.cpp |
Uses the helper for bounded conversions. |
CHANGELOG.md |
Documents BOM-like character preservation. |
Review details
Suppressed comments (1)
mssql_python/pybind/fetch_text.hpp:14
PY_LITTLE_ENDIANis an endian tag value, not a boolean predicate (CPython defines it as a nonzero constant such as 1234). Consequently this expression selects-1on both little- and big-endian builds, so the helper would decode every buffer as little-endian on a big-endian target. ComparePY_BYTE_ORDERwithPY_LITTLE_ENDIAN(or use an equivalent compile-time endianness check) before choosing the decoder byte order.
int byteorder = PY_LITTLE_ENDIAN ? -1 : 1;
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Share nonthrowing native-endian decoding with the two Unix batch processors while retaining their existing fallback behavior and the checked row-wise wrapper. Add platform-aware malformed NVARCHAR batch controls. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 17, 2026 12:47
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Batch decoder changes conflict with the stated scope and require correction or updated scope and validation.
Review details
Suppressed comments (2)
mssql_python/pybind/ddbc_bindings.h:598
- This hunk also changes the batch
ProcessChardecoder from BOM-autodetecting mode to explicit native-endian mode, so it changesfetchmany()/fetchall()behavior outside the stated scope of replacing only the two boundedSQLGetData_wrapexpressions while leaving batch conversion unchanged. Either keep the batch path unchanged or update the PR description and validation/scope claims to explicitly include this behavior change.
PyObject* pyStr =
FetchText::decode_utf16_native(reinterpret_cast<const char*>(wcharData),
numCharsInData * sizeof(SQLWCHAR));
mssql_python/pybind/ddbc_bindings.h:712
- This hunk likewise changes the batch
ProcessWChardecoder, even though the PR description says batch conversion remains unchanged and only the two boundedSQLGetData_wrapexpressions are being replaced. This changes boundedfetchmany()/fetchall()semantics and should either be reverted here or explicitly added to the documented scope and cross-platform validation plan.
SQLWCHAR* wcharData = &buffers.wcharBuffers[col - 1][rowIdx * colInfo->fetchBufferSize];
PyObject* pyStr = FetchText::decode_utf16_native(
reinterpret_cast<const char*>(wcharData), numCharsInData * sizeof(SQLWCHAR));
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
📊 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.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 64.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 77.7%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.py_type_cache.hpp: 91.6%🔗 Quick Links
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
This pull request improves the handling of bounded text columns fetched as UTF-16, ensuring that leading U+FEFF and U+FFFE code points are preserved as payload rather than being treated as byte-order markers (BOMs). This change affects row-wise fetching methods and batch decoding on Linux/macOS, and is covered by new regression tests. The implementation introduces a new utility for consistent UTF-16 decoding and updates the codebase to use it.
Bounded text payload handling:
fetchone(),fetchmany(), andfetchall()results, including when bounded columns are fetched alongside a MAX column.tests/test_017_fetch_bounded_text.pyto verify correct handling of BOMs, surrogate pairs, and payload fidelity for bounded text columns.Decoding implementation changes:
fetch_text.hpputility withFetchText::decode_utf16_nativeandFetchText::from_utf16_nativefor correct, platform-consistent UTF-16 decoding that treats leading BOM-like code points as payload.ddbc_bindings.cppandddbc_bindings.hto use the new decoding utility instead of direct calls toPyUnicode_DecodeUTF16, ensuring consistent handling across the codebase.