- Notifications
You must be signed in to change notification settings - Fork27
Description
Describe the bug
When usingsetDecimalSeparator() to change the decimal separator from the default. (period) to any other character (e.g.,, for European locale), DECIMAL values retrieved viabulk fetch operations (fetchall(),fetchmany() with large result sets) becomeNone, resulting in data loss.
Critical Finding: The bug only affects thebulk/columnar fetch path in the C++ layer. Single-row fetches viafetchone() work correctly because they use a different code path that doesn't apply the separator during parsing.
The decimal separator setting is intended to affectdisplay formatting only (inRow.__str__()), but the bulk fetch implementation incorrectly applies it duringparsing of numeric values from the database.
Exception/Error:
No exception is raised. DECIMAL values silently becomeNone in bulk fetch operations.
Expected: Decimal('1234.56')Got: NoneTo reproduce
Bug appears ONLY with bulk fetch operations (fetchall(),fetchmany()). Single-rowfetchone() works correctly.
Complete reproduction code:
frommssql_pythonimportconnect,setDecimalSeparator,getDecimalSeparatorimportosfromdecimalimportDecimalconn_str=os.getenv("DB_CONNECTION_STRING")conn=connect(conn_str)cursor=conn.cursor()# Create temp table with multiple rowscursor.execute("DROP TABLE IF EXISTS #test_decimal")cursor.execute(""" CREATE TABLE #test_decimal ( id INT, price DECIMAL(10, 2) )""")# Insert test dataforiinrange(10):cursor.execute(f"INSERT INTO #test_decimal VALUES ({i},{1234.56+i})")# Test 1: Bulk fetch with default separator worksprint("Test 1: Bulk fetch with default separator")setDecimalSeparator('.')cursor.execute("SELECT id, price FROM #test_decimal ORDER BY id")rows=cursor.fetchall()print(f"Fetched{len(rows)} rows")print(f"First row:{rows[0][1]}")# Output: Decimal('1234.56') ✅print(f"Type:{type(rows[0][1])}")# Test 2: Bulk fetch with comma separator - BUG!print("\nTest 2: Bulk fetch with comma separator")setDecimalSeparator(',')cursor.execute("SELECT id, price FROM #test_decimal ORDER BY id")rows=cursor.fetchall()print(f"Fetched{len(rows)} rows")print(f"First row:{rows[0][1]}")# Output: None ❌ BUG!print(f"Type:{type(rows[0][1])}")print(f"Expected: Decimal('1234.56')")print(f"Got:{rows[0][1]}")# Test 3: fetchone() works correctly (different code path)print("\nTest 3: fetchone() with comma separator (works!)")setDecimalSeparator(',')cursor.execute("SELECT CAST(9876.54 AS DECIMAL(10, 2)) AS price")row=cursor.fetchone()print(f"Result:{row[0]}")# Output: Decimal('9876.54') ✅print(f"Type:{type(row[0])}")# Cleanupcursor.execute("DROP TABLE #test_decimal")cursor.close()conn.close()
Output:
Test 1: Bulk fetch with default separatorFetched 10 rowsFirst row: 1234.56Type: <class 'decimal.Decimal'>Test 2: Bulk fetch with comma separatorFetched 10 rowsFirst row: NoneType: <class 'NoneType'>Expected: Decimal('1234.56')Got: NoneTest 3: fetchone() with comma separator (works!)Result: 9876.54Type: <class 'decimal.Decimal'>Expected behavior
Data Retrieval: DECIMAL values should always be parsed correctly from the database, regardless of the decimal separator setting. The database returns strings like
"1234.56"(with period), and parsing should always use.as the decimal separator.Display Only: The
setDecimalSeparator()setting shouldonly affect the display format when converting Row objects to strings via__str__():setDecimalSeparator(',')row=cursor.fetchone()print(row[0])# Should print: Decimal('1234.56') - actual valueprint(str(row))# Should print: (1234,56) - formatted display
Further technical details
Python version: 3.11.4 (tested), likely affects all versions
SQL Server version: SQL Server 2022, Azure SQL Database (all versions affected)
Operating system: macOS 14.7.1, Windows 11 (cross-platform issue)
mssql-python version: 0.13.0
Root Cause Analysis:
The bug is inmssql_python/pybind/ddbc_bindings.cpp atline 3243-3253 in thebulk/columnar fetch path. This code path is used byfetchall() andfetchmany() for optimized batch fetching.
Buggy Code (lines 3235-3260):
case SQL_DECIMAL:case SQL_NUMERIC: {try { std::stringnumStr(reinterpret_cast<constchar*>( &buffers.charBuffers[col -1][i * MAX_DIGITS_IN_NUMERIC]), buffers.indicators[col -1][i]);// BUG: Applies custom separator BEFORE parsing std::string separator =GetDecimalSeparator();if (separator !=".") {// Replaces '.' with custom separator (e.g., ',')size_t pos = numStr.find('.');if (pos != std::string::npos) { numStr.replace(pos,1, separator);// "1234.56" → "1234,56" } }// BUG: Passes invalid string to Python's Decimal()// Python's Decimal("1234,56") throws exception → caught → returns None row.append(py::module_::import("decimal").attr("Decimal")(numStr)); }catch (const py::error_already_set& e) {LOG("Error converting to decimal: {}", e.what()); row.append(py::none());// Silently returns None on parse error }break;}
Whyfetchone() works: The non-bulk fetch path (lines 2616-2670) doesn't apply the separator during parsing - it passes the string directly toDecimal() without modification.
Files Affected:
mssql_python/pybind/ddbc_bindings.cpplines 3243-3253 (bulk fetch - BUGGY)mssql_python/pybind/ddbc_bindings.cpplines 2616-2670 (single fetch - CORRECT)mssql_python/row.pylines 152-169 (display formatting - CORRECT)
Additional context
This is a critical bug as it causes silent data loss. Users setting the decimal separator for European locales (,) will receiveNone values instead of actual DECIMAL data when usingfetchall() orfetchmany(), potentially causing calculation errors or application failures.
Workaround:
Only use the default. separator (do not callsetDecimalSeparator()), or usefetchone() exclusively (not practical for large result sets).
Proposed Solution
Fix the bulk fetch path to match the non-bulk fetch path behavior:
Changes Required inddbc_bindings.cpp
Location: Lines 3235-3260 (bulk fetch path)
Current buggy code:
case SQL_DECIMAL:case SQL_NUMERIC: {try { std::stringnumStr(reinterpret_cast<constchar*>( &buffers.charBuffers[col -1][i * MAX_DIGITS_IN_NUMERIC]), buffers.indicators[col -1][i]);// BUG: Don't modify string before parsing! std::string separator =GetDecimalSeparator();if (separator !=".") {size_t pos = numStr.find('.');if (pos != std::string::npos) { numStr.replace(pos,1, separator); } } row.append(py::module_::import("decimal").attr("Decimal")(numStr)); }catch (const py::error_already_set& e) {LOG("Error converting to decimal: {}", e.what()); row.append(py::none()); }break;}
Fixed code (remove separator replacement):
case SQL_DECIMAL:case SQL_NUMERIC: {try { std::stringnumStr(reinterpret_cast<constchar*>( &buffers.charBuffers[col -1][i * MAX_DIGITS_IN_NUMERIC]), buffers.indicators[col -1][i]);// FIX: Always parse with '.' separator (database format)// Custom separator only affects display in Row.__str__() row.append(py::module_::import("decimal").attr("Decimal")(numStr)); }catch (const py::error_already_set& e) {LOG("Error converting to decimal: {}", e.what()); row.append(py::none()); }break;}
Implementation Steps
- Remove lines 3243-3253 in
ddbc_bindings.cpp(the separator replacement logic) - Keep the display formatting in
row.py(lines 152-169) - already correct - Add test cases for bulk fetch with custom decimal separators
- Verify that both
fetchone()andfetchall()work consistently
Expected Behavior After Fix
setDecimalSeparator(',')cursor.execute("SELECT CAST(1234.56 AS DECIMAL(10,2))")# Data retrieval: Always returns proper Decimal objectrows=cursor.fetchall()print(rows[0][0])# Decimal('1234.56') ✅print(type(rows[0][0]))# <class 'decimal.Decimal'> ✅# Display formatting: Uses custom separatorprint(str(rows[0]))# (1234,56) ✅ - display only
Testing Checklist
fetchone()with default separator (already works)fetchone()with custom separator (already works)fetchall()with default separator (already works)fetchall()with custom separator (currently broken, will fix)fetchmany()with custom separator (currently broken, will fix)Row.__str__()display formatting with custom separator (already works)- Cross-platform testing (Windows, Linux, macOS)