-
Notifications
You must be signed in to change notification settings - Fork 87
Shopspring decimal support #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
base: main
Are you sure you want to change the base?
Conversation
…ldb into shopspring-decimal-support
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #287 +/- ##
==========================================
+ Coverage 75.27% 75.48% +0.20%
==========================================
Files 33 34 +1
Lines 6500 6579 +79
==========================================
+ Hits 4893 4966 +73
- Misses 1323 1326 +3
- Partials 284 287 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 PR adds support for shopspring.Decimal and NullDecimal types in the go-mssqldb driver, enabling seamless integration with SQL Server's DECIMAL and MONEY data types. The implementation provides comprehensive support for queries, input/output parameters, table-valued parameters (TVP), and bulk copy operations.
Key changes include:
- Integration of shopspring/decimal package for precise decimal arithmetic
- Introduction of Money[D] generic type supporting both Decimal and NullDecimal
- Complete test coverage for all decimal/money operations including edge cases
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Adds shopspring/decimal dependency |
| money.go | Introduces Money[D] generic type with Value() and Scan() methods |
| mssql.go | Adds makeParam support for decimal types |
| mssql_go19.go | Implements makeMoneyParam and extends makeParamExtra for decimal handling |
| bulkcopy.go | Adds bulk copy support for Money types and decimal string parsing |
| queries_test.go | Adds comprehensive test coverage for decimal parameter operations |
| queries_go19_test.go | Extensive tests for input/output decimal parameters with encoding scenarios |
| tvp_go19_db_test.go | Table-valued parameter tests with decimal support and scaling |
| bulkcopy_test.go | Bulk copy tests for decimal and money types |
| money_test.go | Unit tests for Money type operations and parameter creation |
| internal/decimal/decimal.go | Exposes GetInteger method for internal decimal access |
| README.md | Documents new decimal and money type support |
| func (m *Money[D]) Scan(v any) error { | ||
| scanner, _ := any(&m.Decimal).(sql.Scanner) | ||
|
|
||
| return scanner.Scan(v); |
Copilot
AI
Sep 9, 2025
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.
Remove the unnecessary semicolon at the end of the return statement. Go doesn't require semicolons at the end of statements.
| } | ||
|
|
||
| for i := 0; i < min(len(param1), len(result1)); i++ { |
Copilot
AI
Sep 9, 2025
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.
The decimal rescaling logic is duplicated multiple times in this test file. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
| case decimal.NullDecimal: | ||
| // only null values should be getting here | ||
| res.ti.TypeId = typeNVarChar | ||
| res.buffer = nil | ||
| res.ti.Size = 8000 | ||
| case Money[decimal.NullDecimal]: | ||
| // only null values should be getting here | ||
| res.ti.TypeId = typeNVarChar | ||
| res.buffer = nil | ||
| res.ti.Size = 8000 |
Copilot
AI
Sep 9, 2025
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.
The null handling for decimal.NullDecimal and Money[decimal.NullDecimal] is identical. Consider consolidating this logic to avoid duplication and ensure consistent behavior.
| github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= | ||
| github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= | ||
| github.com/shopspring/decimal v1.4.0 h1:bxl37RwXBklmTi0C79JfXCEBD1cqqHt0bbgBAGFp81k= | ||
| github.com/shopspring/decimal v1.4.0/go.mod h1:gawqmDU56v4yIKSwfBSFip1HdCCXN8/+DMd9qYNcwME= |
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.
it's nice that this module doesn't bring it a bunch of other stuff.
It'd be nice if the driver supported a pluggable type system like we support different network protocols for SQL connections so apps could include the module of their choice to get the implementation. #Resolved
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.
I think there is already some work done in sql library itself, but its still considered experimental, see: golang/go#30870 .
Source code:
% fgrep -A 3 'type decimal interface {' src/database/sql/convert.go
type decimal interface {
decimalDecompose
decimalCompose
}
| "github.com/shopspring/decimal" | ||
| ) | ||
|
|
||
| type Money[D decimal.Decimal|decimal.NullDecimal] struct { |
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.
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.
![]()
|
@apoorvdeshmukh @stuartpa please consider taking a look. |
shopspring.Decimal / NullDecimal mappings to DECIMAL and MONEY, supports queries, in / out parameters, tvp and bulk copy.