Hi Alexey, This is review input re https://github.com/MariaDB/server/commit/02469bdead5753eccb5d70c98a158a07027f4eb2.
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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
