FIX: Inconsistent retrieval of CP1252 encoded data in VARCHAR columns - Windows vs. Linux #468#478
FIX: Inconsistent retrieval of CP1252 encoded data in VARCHAR columns - Windows vs. Linux #468#478subrata-ms wants to merge 2 commits intomainfrom
Conversation
📊 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.pybind.ddbc_bindings.h: 67.8%
mssql_python.pybind.ddbc_bindings.cpp: 69.7%
mssql_python.row.py: 70.5%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.__init__.py: 77.1%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 85.2%🔗 Quick Links
|
subrata-ms
requested review from
bewithgaurav,
Copilot,
gargsaumya,
jahnvi480 and
sumitmsft
There was a problem hiding this comment.
Pull request overview
This PR addresses cross-platform inconsistencies when fetching CP1252-collated VARCHAR data by changing the ODBC C-type used for VARCHAR retrieval on Windows, aiming to ensure consistent Unicode (str) results across Windows and Linux/macOS.
Changes:
- On Windows, fetch/bind
VARCHAR(and relatedSQL_CHARtypes) asSQL_C_WCHARand process via the wide-char path to avoid code page guessing and bytes fallback. - On Linux/macOS, clarify/standardize
SQL_C_CHARbuffer sizing assumptions for UTF-8 expansion. - Add a new CP1252 regression test section covering problematic bytes and multiple fetch paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
mssql_python/pybind/ddbc_bindings.cpp |
Updates SQLGetData_wrap and batch fetch binding/processing to use SQL_C_WCHAR for VARCHAR on Windows; adjusts buffer sizing logic and LOB detection formatting. |
tests/test_013_encoding_decoding.py |
Adds a new CP1252 VARCHAR consistency test suite intended to ensure VARCHAR returns str across platforms and fetch methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| "returning NULL", | ||
| i); | ||
| row.append(py::none()); |
| assert isinstance(value, str), ( | ||
| f"Expected str for CHAR({byte_val}) ({description}) but got " | ||
| f"{type(value).__name__}: {value!r} (platform={sys.platform})." | ||
| ) |
| assert isinstance(value, str), ( | ||
| f"{fetch_method}: expected str but got {type(value).__name__}: " | ||
| f"{value!r} (platform={sys.platform})." | ||
| ) |
| f"Expected str but got {type(value).__name__}: {value!r}. " | ||
| f"Embedded byte 0xAD was not decoded (platform={sys.platform})." | ||
| ) | ||
| assert "hello" in value and "world" in value |
| // Note: wcharEncoding parameter is reserved for future use | ||
| // Currently WCHAR data always uses UTF-16LE for Windows compatibility | ||
| (void)wcharEncoding; // Suppress unused parameter warning | ||
| #if !defined(__APPLE__) && !defined(__linux__) |
| break; | ||
| case SQL_SS_UDT: | ||
| rowSize += (static_cast<SQLLEN>(columnSize) == SQL_NO_TOTAL || columnSize == 0) | ||
| ? SQL_MAX_LOB_SIZE : columnSize; | ||
| ? SQL_MAX_LOB_SIZE | ||
| : columnSize; |
ffelixg
left a comment
ffelixg
left a comment
There was a problem hiding this comment.
Hey, love to see this!
The encoding used by the odbc driver is actually not directly dependent on the operating system, instead it checks which locale is being used and encodes according to that. The difference between the operating systems is that windows uses cp1252 by default and linux uses utf-8. However, you can change the locale and make the driver return data in some other encoding. For example the current test passes on Linux with mssql_python 1.4.0, which shows that you can provoke the same issue on Linux:
def test_locale_varchar_decode_iso885915():
assert locale.getlocale() == ('en_US', 'UTF-8')
# important: change locale BEFORE connecting
locale.setlocale(locale.LC_ALL, 'en_US.iso885915')
connection = connect()
cursor = connection.cursor()
(val,) = cursor.execute("SELECT '€'").fetchone()
# without changing locale, we would get val == '€'
assert val == b'\xa4', val
cursor.close()
connection.close()If we really want to continue to use SQL_C_CHAR for fetching varchar data, even if just on Linux, we would need to note down the current locale when connecting and only fetch SQL_C_CHAR if it is UTF-8 and SQL_C_WCHAR otherwise. This is made more complicated by connection pooling though. Apparently some pooling is already active by default, as followup connections still use the same locale, even if I change it beforehand.
On the flip side, if you do this, you can get rid of the platform specific code, because windows also allows changing the locale to UTF-8, which would enable SQL_C_CHAR there as well.
Work Item / Issue Reference
Summary
This pull request significantly improves cross-platform handling of character encoding for VARCHAR and related types in the
mssql_pythonODBC bindings, ensuring consistent Unicode support and simplifying buffer allocation logic. The changes also update test cases to reflect expanded Unicode and encoding coverage.Platform-specific encoding and buffer allocation:
SQL_C_WCHAR(UTF-16), matching NVARCHAR handling and avoiding code page guessing; on Linux/macOS, VARCHAR columns are fetched asSQL_C_CHAR(UTF-8), with buffer sizes adjusted for potential multi-byte expansion. [1] [2] [3] [4] [5] [6] [7] [8]ProcessWChar), ensuring correct decoding and consistency.LOB and buffer management improvements:
FetchMany_wrapandFetchAll_wrap. [1] [2]Unicode and encoding test coverage:
Minor code cleanup:
These updates ensure robust, consistent Unicode support across platforms and provide a solid foundation for further improvements to encoding/decoding and LOB streaming in the driver.