maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

Hi Alexey,

> revision-id: 0d342ab173cdbf5d35c5f2833c66b6004b9e908e (mariadb-10.5.2-330-g0d342ab173c)
> parent(s): b1ab211dee599eabd9a5b886fafa3adea29ae041
> author: Alexey Botchkov
> committer: Alexey Botchkov
> timestamp: 2020-06-08 14:51:04 +0400
> message:
> 
> MDEV-17399 Add support for JSON_TABLE.
> 
> ha_json_table handler implemented.
> JSON_TABLE() added to SQL syntax.
> 

First, some general input: 

== Assertion with Prepared Statements ==
prepare s from 
'select * from
  json_table(?,
             \'$[*]\' columns( color varchar(100) path \'$.color\',
                          price text path \'$.price\',
                          seq for ordinality
                        )
             ) as T
order by color desc;
'
  mysqld: /home/psergey/dev-git2/10.5-hf-review/sql/item.h:1002: virtual bool Item::fix_fields(THD*, Item**): Assertion `basic_const_item()' failed.

== Default value for TEXT ==

Default value for VARCHAR(100) is SQL NULL, while for TEXT it is empty string.
Is this intentional? If yes, I would like to see the spec of the logic behind
this.

select *
from
  json_table('[{"color": "blue", "price": 50}, {"color": "red", "price": 100}]',
             '$[*]' columns(color varchar(100) path '$.nonexistent',
                            seq for ordinality
                        )
             ) as T;
+-------+------+
| color | seq  |
+-------+------+
| NULL  |    1 |
| NULL  |    2 |
+-------+------+

select *
from
  json_table('[{"color": "blue", "price": 50}, {"color": "red", "price": 100}]',
             '$[*]' columns(color text path '$.nonexistent',
                            seq for ordinality
                        )
             ) as T;
+-------+------+
| color | seq  |
+-------+------+
|       |    1 |
|       |    2 |
+-------+------+

== JSON_TABLE can have a non-unique alias? ==

create table t20(a varchar(100));
insert into t20 values ();

create table t21(a varchar(100));
insert into t21 values ();

select * from t20 as T, t21 as T;
ERROR 1066 (42000): Not unique table/alias: 'T'

Good. This is the expected behaviour.

select *
from
  t20 as T , json_table(T.a,
             '$[*]' columns(color varchar(100) path '$.nonexistent',
                            seq for ordinality
                        )
             ) as T;
Empty set (0.000 sec)

No error. I think this is wrong. Why would JSON_TABLE be allowed to use an
alias that conflicts with another table?

== EXPLAIN doesn't show table function == 

I had requested this in previous review(s): Table function use should be shown
in EXPLAIN [FORMAT=JSON].

Here's how MySQL does it:
https://gist.github.com/spetrunia/257c929b0c04a866faf6428e733999ad

Note that this can be extended to handle other table functions.

if there are no other ideas, let's use the same syntax.

== Comments ==

I still think the patch doesn't have good comments. Let me get back to you on
this.

> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> index 49cabec9916..3db438ee470 100644
> --- a/sql/share/errmsg-utf8.txt
> +++ b/sql/share/errmsg-utf8.txt
> @@ -7965,3 +7965,7 @@ ER_NOT_ALLOWED_IN_THIS_CONTEXT
>          eng "'%-.128s' is not allowed in this context"
>  ER_DATA_WAS_COMMITED_UNDER_ROLLBACK
>          eng "Engine %s does not support rollback. Changes where commited during rollback call"
> +ER_JSON_TABLE_ERROR_ON_FIELD
> +        eng "Field '%s' can't be set for JSON_TABLE '%s'."

> +ER_JSON_TABLE_DOUBLE_ON_RESPONSE
> +        eng "ON %s response specified more than once in JSON_TABLE definition."

I agree that using this error solves the issue I've complained about, but this
doesn't look like the right way to do it:

* Why not just emit a parser error with YYERROR in this case?
* Better: why not just remove the recursive definition from the grammar?

...
> diff --git a/sql/table_function.cc b/sql/table_function.cc
...
> +
> +/*
> +  handler::print_error has a case statement for error numbers.
> +  This value is (10000) is far out of range and will envoke the
> +  default: case.(Current error range is 120-159 from include/my_base.h)
> + */
> +#define  HA_ERR_INVALID_JSON 10000

So, is this needed anymore?  

On the other hand, the new solution with returning HA_ERR_TABLE_IN_FK_CHECK
doesn't look very good either. 
A plugin could register its own error codes, but this engine is not a plugin.
Let's discuss with Serg and get back to this.

> +static int print_path(String *str, const json_path_t *p)
> +{
> +  return str->append('\"') ||
> +         str->append((const char *) p->s.c_str, p->s.str_end - p->s.c_str) ||
> +         str->append('\"');
> +}
The above doesn't do proper quoting, which is wrong. Testcase:

select *
from
  json_table('[{"co\\\\lor": "blue", "price": 50}]',
             '$[*]' columns( color varchar(100) path '$.co\\\\lor')
             ) as T;
+-------+
| color |
+-------+
| blue  |
+-------+

create view v2 as
select *
from
  json_table('[{"co\\\\lor": "blue", "price": 50}]',
             '$[*]' columns( color varchar(100) path '$.co\\\\lor')
             ) as T;
select * from v2;
+-------+
| color |
+-------+
| NULL  |
+-------+



BR
 Sergei
-- 
Sergei Petrunia, Software Developer
MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog