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?)

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.

> 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 the column was
undefined ans set to the SQL NULL.

Here is the new patch  https://github.com/MariaDB/server/commit/fe0dc6ba769dcb468b37a8c3e3c636c0049f7307

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/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