Improve handling of unsigned and decimal types in JSON #2404
Conversation
…converting to JSON. This is both faster and also prevents us from dropping information when writing to a table and reading it back. (Particularly about whether a value is unsigned, and whether it's a decimal.)
…end the correct value along the wire.
We still return "INTEGER" for a wrapped float that represents a whole number, because the `encoding/json` package always emits floats when parsing strings into JSON. It's not currently possible for us to distinguish between a "1" and "1.0" in a parsed JSON string, so we assume that it's an integer.
| @@ -67,6 +68,24 @@ func (t JsonType) Convert(v interface{}) (doc interface{}, inRange sql.ConvertIn | |||
| if err != nil { | |||
| return nil, sql.OutOfRange, sql.ErrInvalidJson.New(err.Error()) | |||
| } | |||
| case int8: | |||
There was a problem hiding this comment.
I don't think a float case here matters for correctness, because in the default case floats and doubles will both get serialized and then parsed back as doubles, which is the correct behavior.
But adding a case here avoids doing more unnecessary serialization though, so I added it.
Since the observed behavior should be the same, I didn't add a test.
|
|
||
| var InvalidJsonArgument = errors.NewKind("invalid data type for JSON data in argument 1 to function json_value; a JSON string or JSON type is required") | ||
|
|
||
| func GetJSONOrCoercibleString(js interface{}) (jsonData interface{}, err error) { |
There was a problem hiding this comment.
the naming implies the argument shape but doesn't give expectations of the return value. MapFromJSONOrCoercibleString? Maybe docstring?
There was a problem hiding this comment.
It's not necessarily a map, since it could also any other value that can be represented in JSON.
I renamed it to GetJSONFromWrapperOrCoercibleString and added a docstring.
| @@ -314,6 +316,7 @@ func writeMarshalledValue(writer io.Writer, val interface{}) error { | |||
| writer.Write([]byte{'"'}) | |||
| return nil | |||
| case json.Marshaler: | |||
| decimal.MarshalJSONWithoutQuotes = true | |||
There was a problem hiding this comment.
would we want to do this on init? seems like we'd want the lifecycle to be predictable
There was a problem hiding this comment.
We probably do want to do it on init, but I wasn't sure where the best place to do that would be. This is the only place where we marshal decimals (I think...) so it should be predictable, but confusing and easy to make unpredictable in the future.
Do you have a suggestion for where I should do this?
There was a problem hiding this comment.
my first thought was top of file in the startup routine func init() {}?
There was a problem hiding this comment.
Ah. Not sure how I missed that. Fixed.
| }, | ||
| }, | ||
| { | ||
| Query: `select JSON_TYPE(CAST("\"321.4\"" AS JSON))`, |
There was a problem hiding this comment.
we've had weird type edge cases with exponentials before, ex 1e2, "1e2", 1.0e2, ...
There was a problem hiding this comment.
Added additional tests.
…sn't affect correctness but should avoid unnecessary parsing.
Fixes #2391
MySQL's JSON type differs from standard JSON in some important ways. It supports types not supported in standard JSON, such as separate types for integers and floats, an unsigned int type, and a decimal type.
Prior to this PR, we would convert values to JSON by using the
encodings/jsonpackage to marshall the value to a JSON string and then unmarshall it to a go map. This is not only slow, but it's incorrect for these additional types.The main purpose of this PR is to add special handling for these types that allow them to be stored in JSON documents. We also avoid generating and parsing JSON in places where it's not actually necessary, and fix bugs where decimals get incorrectly converted into strings, or unsigned ints get converted into signed ints.
Finally, this fixes an issue where we send incorrect bytes for JSON-wrapped decimal values along the wire.