feat: add JSON type support - #69
Conversation
jchavarri
commented
Jul 8, 2025
jchavarri
commented
- Add MYSQL_TYPE_JSON constant to FFI bindings
- Add Json variant to buffer_type and value types
- Implement json binding and field accessor functions
- Add comprehensive JSON tests for various data types
- Support both native JSON and string compatibility modes
- Tested against TiDB and MySQL 8.0.23
- Add MYSQL_TYPE_JSON constant to FFI bindings - Add Json variant to buffer_type and value types - Implement json binding and field accessor functions - Add comprehensive JSON tests for various data types - Support both native JSON and string compatibility modes - Tested against TiDB and MySQL 8.0.23
paurkedal
left a comment
paurkedal
left a comment
There was a problem hiding this comment.
Thanks, it's good to cover the JSON SQL type, though I don't think it needs to be exposed in the interface, see my comment below.
| | `String of string | ||
| | `Bytes of bytes | ||
| | `Time of Time.t | ||
| | `Json of string |
There was a problem hiding this comment.
Would it not be better to use the `String case for JSON, as well? I didn't write the original code, but the intention seem to me that the value type only contains the cases which have different representation on the OCaml side, e.g. omitting the various integer variants.
| let json field = | ||
| match value field with | ||
| | `Json j -> j | ||
| | _ -> err field ~info:"a json value" | ||
|
|
There was a problem hiding this comment.
In the test case you take into account the possibility that the database server returns a string (or bytes, as I've when testing). That would be needed here, as well. But if you agree to the above, this definition can be omitted.
| let json_opt field = | ||
| match value field with | ||
| | `Json j -> Some j | ||
| | `Null -> None | ||
| | _ -> err field ~info:"a nullable json value" |
| | `Json j -> j | ||
| | `String s -> s (* TiDB/MySQL might return as string *) |
There was a problem hiding this comment.
I'm also seeing `Bytes here, using MariaDB client/server version 10.11.13.
get rid of `Json constr
|
@paurkedal Thanks for the review, I think all the comments were tackled. Please let me know if anything else is missing. |
|
I added a I need to think about it a bit more. Let me know if you have any thoughts. |
This also reintroduce the `JSON case internally, making the code consistent with other cases.
|
Can you take a look at my latest commit to see if you agree with my implementation? This follows the current pattern for other SQL types, but it means we receive the JSON as Shall we preserving the current commit history when I merge? |
|
One more thing, can you re-test against TiDB and MySQL 8.0.23? |
|
You're right about this. After checking the MariaDB docs, JSON is just an alias for LONGTEXT COLLATE utf8mb4_bin, so using bytes makes sense. Also retested for TiDB and MySQL 8.0.23, together with your last commit, everything works P.S |
I don't think we have a strong opinion. Whichever way seems better to you. |
|
Great, thanks for the contribution! |
3 participants
