Fix numeric precision loss in JSON parsing #28916
Conversation
e4fb4d1 to
faddc7d
Compare
|
Added benchmark. Results visualized here: Text results BeforeAfter |
|
I think these results address @dain 's concern (#28882 (comment)). |
88d88a4 to
5992621
Compare
| private static final JsonMapper SORTED_MAPPER = new JsonMapperProvider().get() | ||
| .rebuild() | ||
| .configure(ORDER_MAP_ENTRIES_BY_KEYS, true) | ||
| .configure(USE_BIG_DECIMAL_FOR_FLOATS, true) |
There was a problem hiding this comment.
this isn't backward compatible, right?
There was a problem hiding this comment.
If you look at the tests it is definitely a behavior change. We could call this a fix or a backwards incompatible change. I think of it as a fix, but I think it makes sense to mark this as a backwards incompatible change in the release notes so we properly warn users.
There was a problem hiding this comment.
Yeah, we should call it out, but fix itself is a correct thing
|
This is really interesting behavior. So in Jackson, if I don't have this something like getText returns values that have been round-tripped through double? |
No. I believe getText works as expected. However, in Trino, most JSON values come from |
5992621 to
a0b1633
Compare
|
( just rebased after #28932 merged, no other changes ) |
I think I brought this up elsewhere. I think for json cast to string, we should just preserve the number as is without normalizing. I think this is really want users want, and it happens to be cheaper. |
cast to string is not being changed here. It's covered by however, this PR can be viewed as prerequisite. |
dain
left a comment
There was a problem hiding this comment.
Looks good but drop the empty commit at the end
9a09c50 to
01dc0d0
Compare
|
Rebased to avoid logical merge conflicts after this merged |
The general purpose `jsonParse` utility was lossy when it comes to numbers containing a decimal point. This affected `json_parse` SQL function, `JSON` SQL type constructor and connectors which use `jsonParse` to canonicalize JSON representation on remote data read (e.g. PostgreSQL).
01dc0d0 to
0a4450b
Compare
|
Resolved logical merge conflicts (tests only) |
json_parse, JSON constructor and connectors #28867release notes