maria-developers team mailing list archive

[Alexey Botchkov <holyfoot@xxxxxxxxxxx>]

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@xxxxxxxxxxx> 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
>
>
>