FIX: Remove unused decimal separator code and add regression tests

[Chetan-Kansal]

Work Item / Issue Reference

GitHub Issue: #295

---

Summary

Removes misleading unused decimal-separator code from the bulk fetch path and adds a regression test to ensure DECIMAL values are parsed correctly regardless of setDecimalSeparator().

[Chetan-Kansal]

@microsoft-github-policy-service agree

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 100%

🎯 Overall Coverage 76%

📈 Total Lines Covered:5433 out of 7115 📁 Project:mssql-python

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

---

📋 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: 69.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: 84.1%

---

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Gaurav Sharma (bewithgaurav)]

Thank you Chetan-Kansal for the PR!

Looking closely with the codebase, the core issue #295 was already fixed in PR #320, which introduced this comment in the code at lines 3142-3147:

// Always use standard decimal point for Python Decimal parsing// The decimal separator only affects display formatting, not parsing
py::object decimalObj = PythonObjectCache::get_decimal_class()(py::str(cnum, safeLen));

This ensures DECIMAL values are parsed correctly regardless of the setDecimalSeparator() setting.

However, your PR performs important cleanup and adds a focused regression test:

• Removes unused dead code: The decimalSeparator variables at lines 2902 and 3625 were declared but never used
• Adds regression tests: Ensures the fix stays in place and prevents future regressions

Suggested Title Change

Since this is cleanup + testing rather than the actual fix, I suggest updating the PR title to be more accurate:

Current: "FIX: clarify decimal parsing in bulk fetch and add regression test"

Suggested: "FIX: Remove unused decimal separator code and add regression tests"

• The code changes look correct (removing dead code)
• The tests are valuable (preventing regression)

Great work on the PR and adding comprehensive tests! This will help ensure the fix remains stable.

[Chetan-Kansal]

Hi Gaurav Sir,
The coverage report job appears to be timing out while waiting for the Azure DevOps pipeline to start.
It looks like no ADO build is being triggered for PR #384, so the GitHub workflow eventually times out.
Please let me know if the ADO pipeline needs manual approval or if there’s anything I should do from my side.

[Gaurav Sharma (bewithgaurav)]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Gaurav Sharma (bewithgaurav)]

lgtm, thanks for contributing!