-
Notifications
You must be signed in to change notification settings - Fork 26
FIX: Fix precision loss when binding large Decimal values to SQL_NUMERIC_STRUCT #287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request fixes precision loss when binding large Decimal values to SQL_NUMERIC_STRUCT by increasing the maximum supported precision from 15 to 38 digits and changing the internal representation from a 64-bit integer to a 16-byte binary format compatible with SQL Server's numeric storage.
- Increased maximum precision limit from 15 to 38 digits to match SQL Server's capabilities
- Changed C++ NumericData struct to use binary string representation instead of 64-bit integer
- Replaced old numeric tests with comprehensive test suite covering edge cases and high-precision values
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mssql_python/cursor.py | Updated _get_numeric_data method to support 38-digit precision and convert decimals to 16-byte binary format |
| mssql_python/pybind/ddbc_bindings.cpp | Modified NumericData struct to use string instead of uint64_t and updated parameter binding logic |
| tests/test_004_cursor.py | Removed old numeric tests and added comprehensive test suite for various numeric scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
94ccdc3 to
cbb94b3
Compare
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/cursor.pyLines 230-244 230 # strip decimal point from param & convert the significant digits to integer
231 # Ex: 12.34 ---> 1234
232 int_str = ''.join(str(d) for d in digits_tuple)
233 if exponent > 0:
! 234 int_str = int_str + ('0' * exponent)
235 elif exponent < 0:
236 if -exponent > num_digits:
! 237 int_str = ('0' * (-exponent - num_digits)) + int_str
238
239 if int_str == '':
! 240 int_str = '0'
241
242 # Convert decimal base-10 string to python int, then to 16 little-endian bytes
243 big_int = int(int_str)
244 byte_array = bytearray(16) # SQL_MAX_NUMERIC_LENmssql_python/pybind/ddbc_bindings.cppLines 62-76 62
63 NumericData() : precision(0), scale(0), sign(0), val(SQL_MAX_NUMERIC_LEN, '\0') {}
64
65 NumericData(SQLCHAR precision, SQLSCHAR scale, SQLCHAR sign, const std::string& valueBytes)
! 66 : precision(precision), scale(scale), sign(sign), val(SQL_MAX_NUMERIC_LEN, '\0') {
! 67 if (valueBytes.size() > SQL_MAX_NUMERIC_LEN) {
! 68 throw std::runtime_error("NumericData valueBytes size exceeds SQL_MAX_NUMERIC_LEN (16)");
! 69 }
70 // Copy binary data to buffer, remaining bytes stay zero-padded
! 71 std::memcpy(&val[0], valueBytes.data(), valueBytes.size());
! 72 }
73 };
74
75 // Struct to hold the DateTimeOffset structure
76 struct DateTimeOffsetLines 2058-2076 2058 if (!py::isinstance<NumericData>(element)) {
2059 throw std::runtime_error(MakeParamMismatchErrorStr(info.paramCType, paramIndex));
2060 }
2061 NumericData decimalParam = element.cast<NumericData>();
! 2062 LOG("Received numeric parameter at [%zu]: precision=%d, scale=%d, sign=%d, val=%s",
! 2063 i, decimalParam.precision, decimalParam.scale, decimalParam.sign, decimalParam.val.c_str());
! 2064 SQL_NUMERIC_STRUCT& target = numericArray[i];
! 2065 std::memset(&target, 0, sizeof(SQL_NUMERIC_STRUCT));
! 2066 target.precision = decimalParam.precision;
! 2067 target.scale = decimalParam.scale;
! 2068 target.sign = decimalParam.sign;
! 2069 size_t copyLen = std::min(decimalParam.val.size(), sizeof(target.val));
! 2070 if (copyLen > 0) {
! 2071 std::memcpy(target.val, decimalParam.val.data(), copyLen);
! 2072 }
2073 strLenOrIndArray[i] = sizeof(SQL_NUMERIC_STRUCT);
2074 }
2075 dataPtr = numericArray;
2076 bufferLength = sizeof(SQL_NUMERIC_STRUCT);📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.connection.connection.cpp: 68.3%
mssql_python.ddbc_bindings.py: 68.5%
mssql_python.pybind.ddbc_bindings.cpp: 70.5%
mssql_python.cursor.py: 81.5%
mssql_python.connection.py: 81.7%
mssql_python.helpers.py: 84.7%
mssql_python.auth.py: 85.3%
mssql_python.type.py: 86.8%
mssql_python.pooling.py: 87.5%
mssql_python.exceptions.py: 90.4%🔗 Quick Links
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add tests, pls double check codecoverage - if still uncovered let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
ae9ce0c to
ba016ba
Compare
ba016ba to
9d6e6bb
Compare
3c6100e to
0480cb9
Compare
086279d to
926ddd5
Compare
Work Item / Issue Reference
Summary
This pull request significantly improves the handling of SQL Server NUMERIC/DECIMAL values in both the Python and C++ layers, addressing precision, scale, and binary representation for high-precision decimals. It also introduces a comprehensive suite of tests to validate numeric roundtrip, edge cases, and boundary conditions. The changes ensure compliance with SQL Server's maximum precision (38 digits), robust conversion between Python decimals and SQL binary formats, and better test coverage for numeric types.
Numeric Data Handling Improvements
_get_numeric_datamethod incursor.pynow correctly calculates the binary representation of decimal values, supporting up to 38 digits of precision, and constructs the byte array for SQL Server compatibility. The restriction on precision is raised from 15 to 38 digits. [1] [2] [3]NumericDatastruct now stores the value as a binary string (16 bytes) instead of a 64-bit integer, allowing support for high-precision numerics. Related memory handling is updated for parameter binding. [1] [2] [3] [4] [5]Test Suite Expansion and Refactoring
These changes collectively make the library more robust and compliant with SQL Server's numeric type requirements, and the expanded tests will help catch future regressions.