FEAT: Adding getDecimalSeperator and setDecimalSeperator as global functions #188
Conversation
sumitmsft
left a comment
sumitmsft
left a comment
There was a problem hiding this comment.
Global state in multi-threaded environment
The current implementation uses global state (g_decimalSeparator) without considering thread safety. Given that this library is likely to be used in multi-threaded applications (web servers, concurrent data processing), this could lead to:
Data corruption: Concurrent string operations on g_decimalSeparator
Inconsistent behavior: Different threads seeing different separator values
Race conditions: Database operations using incorrect separators
Suggested Solutions:
Use std::atomicstd::string for the global variable
Add mutex protection around read/write operations
Consider thread-local storage for separator configuration
Encapsulate separator in a thread-safe configuration object/context
|
@jahnvi480 |
github-actions
bot
added
pr-size: large
github-actions
bot
added
pr-size: large
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for configuring the decimal separator used when parsing and displaying NUMERIC/DECIMAL values in the mssql_python package. It introduces new Python and C++ APIs to control the decimal separator, ensures the separator is respected in string representations, and includes comprehensive tests to verify the new functionality.
- Added
setDecimalSeparatorandgetDecimalSeparatorfunctions with input validation and C++ layer integration - Updated data handling logic to use the configured decimal separator when converting NUMERIC/DECIMAL database values
- Modified the Row.str method to apply the custom decimal separator for display formatting
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mssql_python/init.py | Adds global decimal separator functions with validation and C++ integration |
| mssql_python/pybind/ddbc_bindings.h | Defines thread-safe decimal separator accessor class and helper functions |
| mssql_python/pybind/ddbc_bindings.cpp | Implements decimal separator functionality in C++ data fetching logic |
| mssql_python/row.py | Updates Row.str method to use custom decimal separator for display |
| mssql_python/cursor.py | Minor formatting change (extra blank line) |
| tests/test_001_globals.py | Comprehensive tests for decimal separator functionality including thread safety |
| tests/test_004_cursor.py | Database operation tests for decimal separator with string formatting validation |
Comments suppressed due to low confidence (1)
mssql_python/pybind/ddbc_bindings.cpp:1
- The NULL data check was removed but the try-catch block doesn't handle the SQL_NULL_DATA case. This could cause issues when the database returns NULL values for NUMERIC/DECIMAL columns.
// Copyright (c) Microsoft Corporation.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
github-actions
bot
added
pr-size: large
github-actions
bot
added
pr-size: large
Work Item / Issue Reference
Summary
This pull request adds support for configuring the decimal separator used when parsing and displaying NUMERIC/DECIMAL values in the
mssql_pythonpackage. It introduces new Python and C++ APIs to control the decimal separator, ensures the separator is respected in string representations, and includes comprehensive tests to verify the new functionality.Decimal separator configuration and propagation:
setDecimalSeparatorandgetDecimalSeparatorfunctions inmssql_python/__init__.pyto allow users to set and retrieve the decimal separator character, with input validation and propagation to the C++ layer (DDBCSetDecimalSeparator).g_decimalSeparatorvariable and theDDBCSetDecimalSeparatorfunction in C++ (ddbc_bindings.cppandddbc_bindings.h) to store and update the separator, and exposed this setter to Python via pybind11.Integration with data handling and display:
Decimalobjects, ensuring correct parsing and formatting.Row.__str__method to use the current decimal separator when displaying decimal values, so string representations reflect user configuration.Testing:
tests/test_001_globals.pyandtests/test_004_cursor.pyto cover the new decimal separator functionality, including validation, database operations, string formatting, and ensuring calculations are unaffected by the separator setting.These changes make the decimal separator fully configurable and ensure consistent behavior across both parsing and display of decimal values.