FIX: Preserve leading BOM-like characters in bounded text fetches - #793
Merged
Merged
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
|
PR Performance ReportThis PR consistently slows common table expression queries on macOS / SQL Server 2022 by 29.2%.
The largest recorded phase increases for these tasks are shown below. Phase timings are supporting evidence, not root-cause proof. Coverage: 5 of 5 environments completed. Advisory result; does not block merging.
Affected phases and call countsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. macOS / SQL Server 2022SELECT queries: ddbc::SQLExecDirect_wrap +4.910 ms; py::execute::cpp_call +4.909 ms; ddbc::SQLGetAllDiagRecords +0.213 ms. All database tasks and timingsWindows / SQL Server 2022
Windows / SQL Server 2025
macOS / SQL Server 2022
macOS / SQL Server 2025
Linux / SQL Server 2022
Build, commits and measurement detailsPR head:
A consistent slowdown requires more than 20% median paired slowdown, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold. An inconsistent slowdown crosses the first two thresholds without enough pair agreement. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 18, 2026 07:48
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The regression test incorrectly treats empty strings as SQL NULL, causing empty-string cases to fail before exercising the decoder.
Review details
Suppressed comments (1)
tests/test_017_fetch_bounded_text.py:61
UNICODE('')returns0in SQL Server, while this expression recordsNonefor bothNoneand"". As a result, every parametrized case containing the empty string will fail the independent evidence assertion before exercising the decoder. Distinguish SQL NULL from an empty payload when constructingwanted_evidence.
ord(payload[0]) if payload else None,
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Sumit Sarabhai (sumitmsft)
approved these changes
Sep 18, 2026
Jahnvi Thakkar (jahnvi480)
enabled auto-merge (squash)
September 18, 2026 08:43
Gaurav Sharma (bewithgaurav)
approved these changes
Sep 18, 2026
This was referenced Sep 19, 2026
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.