-
Notifications
You must be signed in to change notification settings - Fork 500
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
Support for 'money' type in bulk import #430
base: master
Are you sure you want to change the base?
Conversation
The money type was not supported. Implement it here as an int64. I figured out the permutation of bytes by looking at decodeMoney in types.go.
Appears the build is ok except for one SQL Server version that wasn't available. I don't know how to restart (if it is a transient problem) and assume I can't fix the CI pipeline.. |
The error in pipeline is
...so it doesn't seem to indicate an actual compatability problem, just a failed CI run.. |
Codecov Report
@@ Coverage Diff @@
## master #430 +/- ##
==========================================
+ Coverage 68.09% 68.28% +0.19%
==========================================
Files 21 21
Lines 5093 5121 +28
==========================================
+ Hits 3468 3497 +29
+ Misses 1412 1410 -2
- Partials 213 214 +1
Continue to review full report at Codecov.
|
Looks like it passed after retry. I will try to look at this CR today. |
I think it would be pretty non intuitive to have implicit conversion from 12345 to 1.2345. I would like to hear other opinions on that. |
I tried to support mssqldb.Money{...} but it isnt passed through database/sql to the driver. Strings?... I am thinking perhaps one needs another API than database/sql for bulk import, a way of somehow getting hold of the driver Conn from the *sql.Tx. But could not find it.. |
It is the same problem in the open numeric/decimal bulk insert PR. Using float64 should at least be out of the question IMO. (I do not wish to use float64 for numeric either) So either a) int64 or b) strings or c) do not go through database/sql? |
I parse it as a string and repack it. case typeMoney, typeMoney4, typeMoneyN:
|
I am super late on this review. I'll try to get to this tomorrow. |
I like the code, I agree with the objection raised. I need to figure out decimals, period. |
I made a proposal that would help this effort: |
I would like to handle money/decimals by checking for either the *apd.Decimal type or the DecimalDecompose type. I don't want to do a pre-multiplied int type. |
I would have done that if I could figure out how. Seems like database/sql will only pass primitive types down to the driver; I couldn't find a way around that when I wrote this. |
In go1.13 database/sql will pass down and accept the decimal decompose interface. I'd like to structure future decimal uses around that if possible. |
What is the current status regarding decimal and database/sql? I tried to google a bit but wasn't easy to find the answer. Is it now possible for us to re-implement this in a more standard way and submit it upstream? |
|
No, I think I got that wrong. IIUC the support that is currently present in go may be reverted in the future since it's experimental and the proposal is not accepted?.. |
The money type was not supported for bulk insert. I took the simple way out and implemented it as an int64, so you need to pass an int64 (which fits very well for our usecase, where we have a Money type that wraps int64).
I figured out the permutation of bytes by looking at decodeMoney in types.go.