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