maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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