Fix bulkcopy of DateTimeOffsets and precise Decimals

[jwebb]

• Converting numeric types via float64 risks losing precision, so add direct conversions for ints and strings
• DateTimeOffset values were being rejected by the server due to a typo in the code
• Also support dates/times expressed as strings for consistency with regular inserts, and make the 'invalid type' errors more helpful

[codecov]

Codecov Report

Merging #404 into master will decrease coverage by 0.03%.
The diff coverage is 42.85%.

@@            Coverage Diff            @@
##           master    #404      +/-   ##
=========================================
- Coverage   68.93%   68.9%   -0.04%     
=========================================
  Files          22      22              
  Lines        5009    5033      +24     
=========================================
+ Hits         3453    3468      +15     
- Misses       1350    1359       +9     
  Partials      206     206

Impacted Files	Coverage Δ
bulkcopy.go	57.74% <42.85%> (+0.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd40567...050b152. Read the comment docs.

[kardianos]

Overall, I like what you've done here.

The only think I'm unsure of is if we should do something different for decimals altogether, like use github.com/cockroachdb/apd.

Opinion @denisenkom ?

[denisenkom]

Looks good to me.

As for decimals, do you think we could use rational type (Rat) from math/big package? Decimal is a subset of rational so conversion from decimal to rational is fine but conversion back may not work if rational is not decimal.

[jwebb]

Agree this could do with a larger rework. The [4]int32 at present is just adding complication with no benefit, as all output conversions go via big.Int anyway. The simplistic float64 conversions also have undesirable properties (e.g. round-tripping 1.3 will become 1.2999999999999999245025280). Go provides Grisu3/Dragon4 implementations for this reason.

Thing is, it's not obvious to me that this library needs any sort of decimal type at all. It's public, but it doesn't look like something a client would ever use? Unless there are future plans? Right now AFAICT it's only used momentarily for conversions, and there's very little sharing of code paths. Given that, it would probably be better to implement each of those conversions as a function using whatever library facilities are most appropriate, and ditch the intermediate representation.

(But I still think this PR should be merged in the meantime, as it fixes observable issues.)

[SQLServerIO]

I am using github.com/shopspring/decimal to handle all my decimal needs.

[dagss]

Nice work I'd really like this in..

NOTE: When implementing #430 I first tried introducing a dedicated mssql.Money type for the bulk import. However, the golang database/sql package didn't like that -- it seems that arguments has to be converted to primitive types before they hit the driver.

So I think this PR would need to work with string -- any decimal type would need to serialize to string before it is allowed to hit this driver code.

If I am right then all the suggestions here about using decimal packages would not work. At least without a significant change in approach (another bulk insert API). (I hope I am wrong though.)

On tests:

I notice that the strategy used for testing conflicts with the one I did in #430 . I did that what I did so that it would be possible to test for bulk import of NULL values, but I can rewrite the test to follow the pattern here if required.

[john8329]

Is it possible to fix at least the datetimeoffset issue? I can see that using datetime2 works when using the bulk insert feature, and I'm currently using it as a workaround.

[denisenkom]

Yes, please send PR which contains fix for datetime2 issue. Or provide details about this issue, preferably by opening issue or linking issue, if one already open.

[jwebb]

This PR already contains the fix. As well as another. Can I please understand why you will not merge it?

I'm happy to rebase, if it's going to be merged.

I'm also happy to spend some time on refactoring to remove the Decimal type entirely as I mentioned previously. But I'd first need some feedback from you that this is a direction you want to go in, and I don't think that reworking internals should hold up a fix for a data corruption issue.

In the meantime, we have been using this branch in production for the past year.

[denisenkom]

This PR is too big. Maybe you can break it down into individual fixes. Lets start with datetime fix. I still don't know what is wrong with it, but with smaller PR I might be able to understand.
There is no good solution for decimals yet, so lets postpone it. I believe newer golang will have necessary functionality, once it is ready we can revisit.

[john8329]

The problem I'm having is that if I use that SQL type for the column definition, the bulk insert operation fails, while if the column uses the datetime2 type everything works.

[jwebb]

Datetimeoffsets are this commit - a trivial typo: bcb7c74

I could PR these separately if necessary, and if it will result in everything going in. But I'm afraid that much as I would prefer to contribute fixes back, I can't reasonably allocate time to work on this further if we're going to have to continue to use our own fork anyway.

I must say I'm a little confused by the position that it is better not to apply a fix for a data corruption bug now (or indeed a year ago!), just because the code might be deleted later if something better comes along. Nothing in this fix would make that work any harder. Easier, in fact, given that it also improves error reporting and test coverage.

[denisenkom]

Ok merged datetimeoffsets fix. I will look more into the reset of this PR.
In general it is better when PR contains single fix, especially when it introduces lots of changes. Otherwise it make is much harder to review.

[denisenkom]

I reviewed the rest of the changes, and I approve this PR. @jwebb can you re-base it?

[jwebb]

Great, thanks!

Rebasing a branch this old was slightly nontrivial, so I'm going to do some more testing before updating this PR (rebase is at https://github.com/XTXMarkets/go-mssqldb/commits/bulk-insert2 if anyone's curious). Will check back in, in a day or so.

[jwebb]

Seems fine. Updated.

[denisenkom]

Thank you. Merged.