test and fix for MysSQL float parsing into float64 when placeholders are used #434
Conversation
| @@ -1149,7 +1149,7 @@ func (rows *binaryRows) readRow(dest []driver.Value) error { | |||
| continue | |||
|
|
|||
| case fieldTypeFloat: | |||
| dest[i] = float64(math.Float32frombits(binary.LittleEndian.Uint32(data[pos : pos+4]))) | |||
| dest[i] = float32(math.Float32frombits(binary.LittleEndian.Uint32(data[pos : pos+4]))) | |||
There was a problem hiding this comment.
Unfortunately the driver spec does not allow to return a float32 as a driver.Value: https://golang.org/pkg/database/sql/driver/#Value
There was a problem hiding this comment.
Huh, it's strange that it works anyway. I suppose what the spec says and the details of the implementation vary a bit.
Looking through some of the standard library sql code, I see that they seem to work around this issue by converting floats to strings and back using strconv.ParseFloat with the bit size of the field it is going into: https://golang.org/src/database/sql/convert.go#L256
Actually, that function is called by sql.Rows.Scan, which means that their spec for Value is actually inaccurate: https://golang.org/src/database/sql/sql.go#L1849
It seems that the doc for driver.Rows https://golang.org/pkg/database/sql/driver/#Rows should be updated to specify that they do handle more types, including float32.
There was a problem hiding this comment.
I suppose I will submit a ticket to the golang project asking for clarification.
There was a problem hiding this comment.
If it works and we have proper tests for it, I would be fine with ignoring the specification (which is rather flawed anyway). We're testing against all Go versions and tip, so we would notice if it breaks in a future version.
There was a problem hiding this comment.
Well, this change fixes the issue I was running into and the tests I added cover both cases and passed on Travis.
I added an issue to the Go repo so they will hopefully fix the documentation to include the full list of types that are actually allowed in returns from drivers.
|
They've updated the docs to remove the restriction on types returned, so this fix is even legit by their standards now. |
|
Perfect, then this PR can be merged. Thanks! |
After writing up #433 I realized it would be an easy fix, so I here it is.