[Maria-developers] MDEV-25875: RE 02469b: MDEV-17399 JSON_TABLE Accept JSON values for the JSON fields
Hi Alexey, This is review input re https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a07027.... First, our workflow dictates that this patch should have its own MDEV entry. I've filed it https://jira.mariadb.org/browse/MDEV-25875. Please use it in commit descriptions. == Updates mixed with another patch == The patch has .result changes like:
@@ -551,10 +551,12 @@ JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT NULL ON ERROR)) jt; ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'NULL ON ERROR)) jt' at line 2 SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON EMPTY)) jt; -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON EMPTY)) jt' at line 2 +x +0 SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON ERROR)) jt; -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON ERROR)) jt' at line 2 +x +NULL SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (x DATE PATH '$.x'
These are obviously from another patch (the one about "accepting non-string literals as default"). Please move them to that patch. == Comments ==
@@ -377,6 +378,25 @@ static void store_json_in_field(Field *f, const json_engine_t *je) }
+static int store_json_in_json(Field *f, json_engine_t *je) +{ Please add a comment for the function. Something like
+ const uchar *from= je->value_begin; + const uchar *to; + + if (json_value_scalar(je)) + to= je->value_end; + else + { + int error; + if ((error= json_skip_level(je))) + return error; + to= je->s.c_str; + } + f->store((const char *) from, (uint32) (to - from), je->s.cs); + return 0; +} + + bool Json_table_nested_path::check_error(const char *str) { if (m_engine.s.error) @@ -541,7 +561,12 @@ int ha_json_table::fill_column_values(THD *thd, uchar * buf, uchar *pos) } else { - if (!(error= !json_value_scalar(&je))) + if (jc->m_format_json) + { + if (!(error= store_json_in_json(*f, &je))) + error= er_handler.errors; Can there be a case when this assignment assigns any value other than 0? (My logic - er_handler.errors already had some error status, why did we call store_json_in_json? If there was an error inside store_json_in_json, we do not get to this assignment, as store_json_in_json() returned non-zero error. Is
"Store the current JSON element pointed by je into field f, as a separate JSON document" The name "store_json_in_json" doesn't look good to me. Do you think store_element_as_json_doc() would be better? this correct?)
+ } + else if (!(error= !json_value_scalar(&je))) { store_json_in_field(*f, &je); error= er_handler.errors; @@ -868,6 +893,10 @@ int Json_table_column::set(THD *thd, enum_type ctype, const LEX_CSTRING &path) anctual content. Not sure though if we should. */ m_path.s.c_str= (const uchar *) path.str; + + if (ctype == PATH) + m_format_json= m_field->type_handler() == &type_handler_json_longtext;
This will probably cause warnings on some compiler. Please wrap into "MY_TEST(...)" or just brackets, "z= (x == y)". == A discrepancy between this and MySQL == I've found this case: select * from json_table('{"foo": null}', '$' columns( jscol json path '$.foo') ) as T; with the above patch, it produces a JSON null value: +-------+ | jscol | +-------+ | null | +-------+ while in MySQL-8 it produces an SQL NULL: +-------+ | jscol | +-------+ | NULL | +-------+ I'm not yet sure what we should do about this. Any thoughts? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
Hello, Sergey!
The name "store_json_in_json" doesn't look good to me. Do you think store_element_as_json_doc() would be better? I decided to rename it as store_value_as_json_doc() The 'element' sounds somewhat unclear to me.
+ if (!(error= store_json_in_json(*f, &je))) + error= er_handler.errors; Can there be a case when this assignment assigns any value other than 0?
Well i hardly can imagine anything else than 0. Not-zero happens when setting a value to a field produces an error, but there's no realistic errors that can happen when we set the confirmed valid JSON to the JSON field. So can probably be removed. I decided to keep it to behave same way as with other field types, and to be more future-prone. Say When we implement the FORMAT JSON.
If there was an error inside store_json_in_json, we do not get to this assignment, as store_json_in_json() returned non-zero error. Is this correct?)
select * from json_table('{"foo": null}', '$' columns( jscol json path '$.foo')) as T; with the above patch, it produces a JSON null value: while in MySQL-8 it produces an SQL NULL: The JSON null seems more correct to me. It's the JSON null in the original document, why to replace it with the SQL NULL? And it can be useful to detect cases when
No. You can look at the similar code around the store_json_in_field(). store_json_in_jso() returns 0 even if the field->store() failed. Then we check the er_handler.errors to check if that sort of failure was important to us. the column was undefined ans set to the SQL NULL. Here is the new patch https://github.com/MariaDB/server/commit/fe0dc6ba769dcb468b37a8c3e3c636c0049... Best regards. HF On Tue, Jun 8, 2021 at 2:55 PM Sergey Petrunia <sergey@mariadb.com> wrote:
Hi Alexey,
This is review input re
https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a07027... .
First, our workflow dictates that this patch should have its own MDEV entry. I've filed it https://jira.mariadb.org/browse/MDEV-25875. Please use it in commit descriptions.
== Updates mixed with another patch ==
The patch has .result changes like:
@@ -551,10 +551,12 @@ JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT NULL ON ERROR)) jt; ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'NULL ON ERROR)) jt' at line 2 SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON EMPTY)) jt; -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON EMPTY)) jt' at line 2 +x +0 SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (x INT PATH '$.x' DEFAULT 0 ON ERROR)) jt; -ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '0 ON ERROR)) jt' at line 2 +x +NULL SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (x DATE PATH '$.x'
These are obviously from another patch (the one about "accepting non-string literals as default"). Please move them to that patch.
== Comments ==
@@ -377,6 +378,25 @@ static void store_json_in_field(Field *f, const json_engine_t *je) }
+static int store_json_in_json(Field *f, json_engine_t *je) +{ Please add a comment for the function. Something like
"Store the current JSON element pointed by je into field f, as a separate JSON document"
The name "store_json_in_json" doesn't look good to me. Do you think store_element_as_json_doc() would be better?
+ const uchar *from= je->value_begin; + const uchar *to; + + if (json_value_scalar(je)) + to= je->value_end; + else + { + int error; + if ((error= json_skip_level(je))) + return error; + to= je->s.c_str; + } + f->store((const char *) from, (uint32) (to - from), je->s.cs); + return 0; +} + + bool Json_table_nested_path::check_error(const char *str) { if (m_engine.s.error) @@ -541,7 +561,12 @@ int ha_json_table::fill_column_values(THD *thd, uchar * buf, uchar *pos) } else { - if (!(error= !json_value_scalar(&je))) + if (jc->m_format_json) + { + if (!(error= store_json_in_json(*f, &je))) + error= er_handler.errors; Can there be a case when this assignment assigns any value other than 0? (My logic - er_handler.errors already had some error status, why did we call store_json_in_json? If there was an error inside store_json_in_json, we do not get to this assignment, as store_json_in_json() returned non-zero error. Is this correct?)
+ } + else if (!(error= !json_value_scalar(&je))) { store_json_in_field(*f, &je); error= er_handler.errors; @@ -868,6 +893,10 @@ int Json_table_column::set(THD *thd, enum_type ctype, const LEX_CSTRING &path) anctual content. Not sure though if we should. */ m_path.s.c_str= (const uchar *) path.str; + + if (ctype == PATH) + m_format_json= m_field->type_handler() == &type_handler_json_longtext;
This will probably cause warnings on some compiler. Please wrap into "MY_TEST(...)" or just brackets, "z= (x == y)".
== A discrepancy between this and MySQL == I've found this case:
select * from json_table('{"foo": null}', '$' columns( jscol json path '$.foo') ) as T;
with the above patch, it produces a JSON null value: +-------+ | jscol | +-------+ | null | +-------+
while in MySQL-8 it produces an SQL NULL: +-------+ | jscol | +-------+ | NULL | +-------+
I'm not yet sure what we should do about this. Any thoughts?
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net
participants (2)
-
Alexey Botchkov -
Sergey Petrunia