SQL/JSON: JSON_TABLE

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
50 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov
On 23.07.2019 16:58, Pavel Stehule wrote:

I got warning

ar crs libpgcommon.a base64.o config_info.o controldata_utils.o d2s.o exec.o f2s.o file_perm.o ip.o keywords.o kwlookup.o link-canary.o md5.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o rmtree.o saslprep.o scram-common.o string.o unicode_norm.o username.o wait_error.>
...skipping...
clauses.c:1076:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
 1076 |   if (ExecEvalJsonNeedsSubTransaction(jsexpr, NULL))
      |   ^~
clauses.c:1078:4: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the ‘if’
 1078 |    return true;
      |    ^~~~~~
gcc -Wall -Wmissing-protot
Fixed in 38th version. Thanks.


Regress tests diff is not empty - see attached file
Unfortunately, this is not reproducible on my machine, but really seems to be 
a bug.


some strange fragments from code:

    deparseExpr(node->arg, context);
-   if (node->relabelformat != COERCE_IMPLICIT_CAST)
+   if (node->relabelformat != COERCE_IMPLICIT_CAST &&
+       node->relabelformat == COERCE_INTERNAL_CAST)

There obviously should be

node->relabelformat != COERCE_INTERNAL_CAST
Fixed in 38th version. Thanks.
Now, "format"  is type_func_name_keyword, so when you use it, then nobody 
can use "format" as column name. It can break lot of application. "format" 
is common name. It is relatively unhappy, and it can touch lot of users.
FORMAT was moved to unreserved_keywords in the 38th version. I remember that
there was conflicts with FORMAT, but now it works as unreserved_keyword.


This patch set (JSON functions & JSON_TABLE) has more tha 20K rows. More, there are more than few features are implemented.

Is possible to better (deeper) isolate these features, please? I have nothing against any implemented feature, but it is hard to work intensively (hard test) on this large patch. JSON_TABLE has only 184kB, can we start with this patch?

SQLJSON_FUNCTIONS has 760kB - it is maybe too much for one feature, one patch.

Patch 0001 is simply a squash of all 7 patches from the thread 
"SQL/JSON: functions".  These patches are preliminary for JSON_TABLE.

Patch 0002 only needs to be review in this thread.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov
Attached 39th version of the patches rebased onto current master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-SQL-JSON-functions-v39.patch.gz (153K) Download Attachment
0002-JSON_TABLE-v39.patch.gz (38K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule
Hi

so 28. 9. 2019 v 3:53 odesílatel Nikita Glukhov <[hidden email]> napsal:
Attached 39th version of the patches rebased onto current master.


Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1

Comments:

* +<->/* Only XMLTABLE and JSON_TABLE are supported currently */

this comment has not sense more. Can be removed. Probably long time there will not be new format like XML or JSON

* there are new 600 lines to parse_clause.c, maybe this code can be placed in new file parse_jsontable.c ? parse_clause.c is pretty long already (json_table has very complex syntax)

*
+<->if (list_length(ci->passing.values) > 0)
+<->{
+<-><-->ListCell   *exprlc;
+<-><-->ListCell   *namelc;
+

It's uncommon usage of list_length function. More common is just "if (ci->passing.values) {}". Is there any reason for list_length?

* I tested some examples that I found on net. It works very well. Minor issues are white chars for json type. Probably json_table should to trim returned values, because after cutting from document, original white chars lost sense. It is not a problem jsonb type, that reduce white chars on input.

I did only simple tests and I didn't find any other issues than white chars problems for json type. I'll continue in some deeper tests. Please, prepare documentation. Without documentation there is not clean what features are supported. I have to do blind testing.

Regards

Pavel







 
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

regression.diffs (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule
Hi


po 30. 9. 2019 v 18:09 odesílatel Pavel Stehule <[hidden email]> napsal:
Hi

so 28. 9. 2019 v 3:53 odesílatel Nikita Glukhov <[hidden email]> napsal:
Attached 39th version of the patches rebased onto current master.


This patch is still pretty big - it is about 6000 lines (without any documentation). I checked the standard - and this patch try to implement

JSON_TABLE as part of T821
Plan clause T824
Plan default clause T838.

Unfortunately for last two features there are few documentation other than standard, and probably other databases doesn't implement these features (I didn't find it in Oracle, MySQL, MSSQL and DB2) . Can be this patch divided by these features? I hope so separate review and commit can increase a chance to merge this code (or the most significant part) in this release.

It is pretty hard to do any deeper review without documentation and without other information sources.

What do you think?

Regards

Pavel


 

Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1

Comments:

* +<->/* Only XMLTABLE and JSON_TABLE are supported currently */

this comment has not sense more. Can be removed. Probably long time there will not be new format like XML or JSON

* there are new 600 lines to parse_clause.c, maybe this code can be placed in new file parse_jsontable.c ? parse_clause.c is pretty long already (json_table has very complex syntax)

*
+<->if (list_length(ci->passing.values) > 0)
+<->{
+<-><-->ListCell   *exprlc;
+<-><-->ListCell   *namelc;
+

It's uncommon usage of list_length function. More common is just "if (ci->passing.values) {}". Is there any reason for list_length?

* I tested some examples that I found on net. It works very well. Minor issues are white chars for json type. Probably json_table should to trim returned values, because after cutting from document, original white chars lost sense. It is not a problem jsonb type, that reduce white chars on input.

I did only simple tests and I didn't find any other issues than white chars problems for json type. I'll continue in some deeper tests. Please, prepare documentation. Without documentation there is not clean what features are supported. I have to do blind testing.

Regards

Pavel







 
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov
Attached 40th version of the patches.


On 19.10.2019 18:31, Pavel Stehule wrote:
This patch is still pretty big - it is about 6000 lines (without any documentation). I checked the standard - and this patch try to implement

JSON_TABLE as part of T821
Plan clause T824
Plan default clause T838.

Unfortunately for last two features there are few documentation other than standard, and probably other databases doesn't implement these features (I didn't find it in Oracle, MySQL, MSSQL and DB2) . Can be this patch divided by these features? I hope so separate review and commit can increase a chance to merge this code (or the most significant part) in this release.

It is pretty hard to do any deeper review without documentation and without other information sources.

What do you think?

I think it is a good idea. So I have split JSON_TABLE patch into three patches, each SQL feature. This really helped to reduce the size of the main patch by about 40%.


On 30.09.2019 19:09, Pavel Stehule wrote:
Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1

Unfortunately, this is still not reproducible on my computer with 64bit Linux and gcc 9.2.1.


Comments:

* +<->/* Only XMLTABLE and JSON_TABLE are supported currently */

this comment has not sense more. Can be removed. Probably long time there will not be new format like XML or JSON
Fixed.

* there are new 600 lines to parse_clause.c, maybe this code can be placed in new file parse_jsontable.c ? parse_clause.c is pretty long already (json_table has very complex syntax)
Ok, the code was moved to parse_jsontable.c.

*
+<->if (list_length(ci->passing.values) > 0)
+<->{
+<-><-->ListCell   *exprlc;
+<-><-->ListCell   *namelc;
+

It's uncommon usage of list_length function. More common is just "if (ci->passing.values) {}". Is there any reason for list_length?

Fixed.

* I tested some examples that I found on net. It works very well. Minor issues are white chars for json type. Probably json_table should to trim returned values, because after cutting from document, original white chars lost sense. It is not a problem jsonb type, that reduce white chars on input.

I did only simple tests and I didn't find any other issues than white chars problems for json type. I'll continue in some deeper tests. Please, prepare documentation. Without documentation there is not clean what features are supported. I have to do blind testing.

I have added some documentation to the patches which has simply been copied from [1], but It still needs some work.

[1] https://www.postgresql.org/message-id/732208d3-56c3-25a4-8f08-3be1d54ad51b%40postgrespro.ru

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-SQL-JSON-functions-v40.patch.gz (164K) Download Attachment
0002-JSON_TABLE-v40.patch.gz (34K) Download Attachment
0003-JSON_TABLE-PLAN-DEFAULT-clause-v40.patch.gz (9K) Download Attachment
0004-JSON_TABLE-PLAN-clause-v40.patch.gz (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule


út 12. 11. 2019 v 1:13 odesílatel Nikita Glukhov <[hidden email]> napsal:
Attached 40th version of the patches.


On 19.10.2019 18:31, Pavel Stehule wrote:
This patch is still pretty big - it is about 6000 lines (without any documentation). I checked the standard - and this patch try to implement

JSON_TABLE as part of T821
Plan clause T824
Plan default clause T838.

Unfortunately for last two features there are few documentation other than standard, and probably other databases doesn't implement these features (I didn't find it in Oracle, MySQL, MSSQL and DB2) . Can be this patch divided by these features? I hope so separate review and commit can increase a chance to merge this code (or the most significant part) in this release.

It is pretty hard to do any deeper review without documentation and without other information sources.

What do you think?

I think it is a good idea. So I have split JSON_TABLE patch into three patches, each SQL feature. This really helped to reduce the size of the main patch by about 40%.

super, I'lll check main patch only now - to time when it will be merged to core


On 30.09.2019 19:09, Pavel Stehule wrote:
Regress tests fails on my comp - intel 64bit Linux, gcc 9.2.1

Unfortunately, this is still not reproducible on my computer with 64bit Linux and gcc 9.2.1.

Maybe it is locale depending issue. My LANG is  LANG=cs_CZ.UTF-8




Comments:

* +<->/* Only XMLTABLE and JSON_TABLE are supported currently */

this comment has not sense more. Can be removed. Probably long time there will not be new format like XML or JSON
Fixed.

* there are new 600 lines to parse_clause.c, maybe this code can be placed in new file parse_jsontable.c ? parse_clause.c is pretty long already (json_table has very complex syntax)
Ok, the code was moved to parse_jsontable.c.

*
+<->if (list_length(ci->passing.values) > 0)
+<->{
+<-><-->ListCell   *exprlc;
+<-><-->ListCell   *namelc;
+

It's uncommon usage of list_length function. More common is just "if (ci->passing.values) {}". Is there any reason for list_length?

Fixed.

* I tested some examples that I found on net. It works very well. Minor issues are white chars for json type. Probably json_table should to trim returned values, because after cutting from document, original white chars lost sense. It is not a problem jsonb type, that reduce white chars on input.

I did only simple tests and I didn't find any other issues than white chars problems for json type. I'll continue in some deeper tests. Please, prepare documentation. Without documentation there is not clean what features are supported. I have to do blind testing.

I have added some documentation to the patches which has simply been copied from [1], but It still needs some work.


ok

Pavel
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule
Hi

please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a problem with patching

Pavel
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov
On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-SQL-JSON-functions-v41.patch.gz (164K) Download Attachment
0002-JSON_TABLE-v41.patch.gz (34K) Download Attachment
0003-JSON_TABLE-PLAN-DEFAULT-clause-v41.patch.gz (9K) Download Attachment
0004-JSON_TABLE-PLAN-clause-v41.patch.gz (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule
Hi

út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <[hidden email]> napsal:
On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

I can say so broken regress tests has related to locales

with czech locale LANG=cs_CZ.UTF8 following two tests fails

     json_sqljson                 ... FAILED      148 ms
     jsonb_sqljson                ... FAILED     3791 ms

The problem is in comparison digits and chars. the result is locale depend.

postgres=# select '10' > 'a' collate "C";
┌──────────┐
│ ?column? │
╞══════════╡
│ f        │
└──────────┘
(1 row)

postgres=# select '10' > 'a' collate "cs_CZ";
┌──────────┐
│ ?column? │
╞══════════╡
│ t        │
└──────────┘
(1 row)
postgres=# select '10' > 'a' collate "en_US";
┌──────────┐
│ ?column? │
╞══════════╡
│ f        │
└──────────┘
(1 row)

Regards

Pavel

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule
In reply to this post by Nikita Glukhov
Hi

út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <[hidden email]> napsal:
On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

I testing functionality - randomly testing some examples that I found on internet.

I found:

a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not. I think should be useful support this clause too.

SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

There is a question how to map boolean result to other data types.

b) When searched value is not scalar, then it returns null. This behave can be suppressed by clause FORMAT Json. I found a different behave, and maybe I found a bug. On MySQL this clause is by default for JSON values (what has sense).

SELECT *
 FROM
      JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
         aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY
        )
      ) AS tt;

It returns null, although it should to return [1,2].

There is another bug maybe. Although there is DEFAULT clause. It returns NULL.

I got correct result when I used FORMAT JSON clause. I think it should be default behave for json and jsonb columns.

Another question - when I used FORMAT JSON clause, then I got syntax error on DEFAULT keyword .. . Is it correct? Why I cannot to use together FORMAT JSON and DEFAULT clauses?

Note - this behave is not described in documentation.

Regards

Pavel


 

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov

On 17.11.2019 13:35, Pavel Stehule wrote:

Hi

út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <[hidden email]> napsal:
On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

I testing functionality - randomly testing some examples that I found on internet.

Thank you for testing JSON_TABLE.
I found:

          
a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not. 
I think should be useful support this clause too. 
SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:
=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$'
                   COLUMNS (
                     a bool PATH 'exists($.a)',
                     b bool PATH 'exists($.b)'
                   ));
 a | b 
---+---
 t | f
(1 row)

But this works as expected only in lax mode.  In strict mode EXISTS() returns 
Unknown that transformed into SQL NULL:

=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$' 
                   COLUMNS (
                     a bool PATH 'strict exists($.a)',
                     b bool PATH 'strict exists($.b)'
                   ));
 a | b 
---+---
 t | 
(1 row)

There is no easy way to return false without external COALESCE(),
DEFAULT false ON ERROR also does not help.  

So, I think it's worth to add EXISTS PATH clause to our implementation.


There is a question how to map boolean result to other data types. 
Now, boolean result can be used in JSON_TABLE columns of bool, int4, text, 
json[b], and other types which have CAST from bool:
SELECT * 
FROM JSON_TABLE('{"a": 1}', '$'
                COLUMNS (
                  a int PATH 'exists($.a)',
                  b text PATH 'exists($.b)'
                ));
 a |   b 
---+-------
 1 | false
(1 row)

b) When searched value is not scalar, then it returns null. This behave can be 
suppressed by clause FORMAT Json. I found a different behave, and maybe I found
a bug.  On MySQL this clause is by default for JSON values (what has sense).
SELECT *
 FROM
      JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
         aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY 
        )
      ) AS tt;
It returns null, although it should to return [1,2]. 
Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values. 
Otherwise an error is thrown, which can be caught by ON ERROR clause. This 
behavior is specified by the standard.

FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
standard does not have any json data types, so I think we can add implicit 
FORMAT JSON for json[b] typed-columns.  But I'm a bit afraid that different 
behavior can be standardized after introduction of json data types in SQL.

There is another bug maybe. Although there is DEFAULT clause. It returns NULL.
ON ERROR should be used if "not a scalar" error needs to be caught:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
        )
    ) AS tt;

     aj     
------------
 {"x": 333}
(1 row)


ON EMPTY catches only empty-result case (for example, non-existent path in 
lax mode):

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY 
        )
    ) AS tt;
     aj     
------------
 {"x": 333}
(1 row)


I got correct result when I used FORMAT JSON clause. 
I think it should be default behave for json and jsonb columns.
I agree that FORMAT JSON could be implicit for json[b] columns.  But I think
there could be one minor problem if we want to verify that returned value is 
scalar.

Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON PATH 'lax $.a' ERROR ON ERROR 
        )
    ) AS tt;
ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item

(This error message with the reference to implicit JSON_VALUE needs to be fixed.)


But with FORMAT JSON we need to construct complex jsonpath with a filter and 
override ON EMPTY behavior:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON FORMAT JSON 
            -- strict mode is mandatory to prevent array unwrapping
            PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
            ERROR ON EMPTY ERROR ON ERROR
        )
    ) AS tt;
ERROR:  no SQL/JSON item

Another question - when I used FORMAT JSON clause, then I got syntax error
on DEFAULT keyword .. . Is it correct? 

Why I cannot to use together FORMAT JSON and DEFAULT clauses?
JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only 
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.

This syntax is specified in the SQL standard:

<JSON table formatted column definition> ::=
  <column name> <data type> FORMAT <JSON representation>
  [ PATH <JSON table column path specification> ]
  [ <JSON table formatted column wrapper behavior> WRAPPER ]
  [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
  [ <JSON table formatted column empty behavior> ON EMPTY ]
  [ <JSON table formatted column error behavior> ON ERROR ]

<JSON table formatted column empty behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT

<JSON table formatted column error behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT


But I also think that DEFAULT clause could be very useful in JSON_QUERY and 
formatted JSON_TABLE columns.

Note - this behave is not described in documentation.
There are references to JSON_QUERY and JSON_VALUE behavior in the definitions 
of JSON_TABLE columns, but their behavior still seems to be unclear.  This 
needs to be fixed.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule


čt 21. 11. 2019 v 17:31 odesílatel Nikita Glukhov <[hidden email]> napsal:

On 17.11.2019 13:35, Pavel Stehule wrote:

Hi

út 12. 11. 2019 v 22:51 odesílatel Nikita Glukhov <[hidden email]> napsal:
On 12.11.2019 20:54, Pavel Stehule wrote:

> Hi
>
> please, can you rebase 0001-SQL-JSON-functions-v40.patch. I have a
> problem with patching
>
> Pavel

Attached 41th version of the patches rebased onto current master.

I testing functionality - randomly testing some examples that I found on internet.

Thank you for testing JSON_TABLE.
I found:

          
a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not. 
I think should be useful support this clause too. 
SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:
=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$'
                   COLUMNS (
                     a bool PATH 'exists($.a)',
                     b bool PATH 'exists($.b)'
                   ));
 a | b 
---+---
 t | f
(1 row)

But this works as expected only in lax mode.  In strict mode EXISTS() returns 
Unknown that transformed into SQL NULL:

=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$' 
                   COLUMNS (
                     a bool PATH 'strict exists($.a)',
                     b bool PATH 'strict exists($.b)'
                   ));
 a | b 
---+---
 t | 
(1 row)

There is no easy way to return false without external COALESCE(),
DEFAULT false ON ERROR also does not help.  

So, I think it's worth to add EXISTS PATH clause to our implementation.


There is a question how to map boolean result to other data types. 
Now, boolean result can be used in JSON_TABLE columns of bool, int4, text, 
json[b], and other types which have CAST from bool:
SELECT * 
FROM JSON_TABLE('{"a": 1}', '$'
                COLUMNS (
                  a int PATH 'exists($.a)',
                  b text PATH 'exists($.b)'
                ));
 a |   b 
---+-------
 1 | false
(1 row)

b) When searched value is not scalar, then it returns null. This behave can be 
suppressed by clause FORMAT Json. I found a different behave, and maybe I found
a bug.  On MySQL this clause is by default for JSON values (what has sense).
SELECT *
 FROM
      JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
         aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY 
        )
      ) AS tt;
It returns null, although it should to return [1,2]. 
Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values. 
Otherwise an error is thrown, which can be caught by ON ERROR clause. This 
behavior is specified by the standard.

FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
standard does not have any json data types, so I think we can add implicit 
FORMAT JSON for json[b] typed-columns.  But I'm a bit afraid that different 
behavior can be standardized after introduction of json data types in SQL.

There is another bug maybe. Although there is DEFAULT clause. It returns NULL.
ON ERROR should be used if "not a scalar" error needs to be caught:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
        )
    ) AS tt;

     aj     
------------
 {"x": 333}
(1 row)


ON EMPTY catches only empty-result case (for example, non-existent path in 
lax mode):

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY 
        )
    ) AS tt;
     aj     
------------
 {"x": 333}
(1 row)


I got correct result when I used FORMAT JSON clause. 
I think it should be default behave for json and jsonb columns.
I agree that FORMAT JSON could be implicit for json[b] columns.  But I think
there could be one minor problem if we want to verify that returned value is 
scalar.

Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON PATH 'lax $.a' ERROR ON ERROR 
        )
    ) AS tt;
ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item

(This error message with the reference to implicit JSON_VALUE needs to be fixed.)


But with FORMAT JSON we need to construct complex jsonpath with a filter and 
override ON EMPTY behavior:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON FORMAT JSON 
            -- strict mode is mandatory to prevent array unwrapping
            PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
            ERROR ON EMPTY ERROR ON ERROR
        )
    ) AS tt;
ERROR:  no SQL/JSON item
please, check the behave of other databases. I think so good conformance with other RDBMS is important. More this method for checking if value is object or not looks little bit scary.

maybe we can implement some functions like JSON_IS_OBJECT(), JSON_IS_ARRAY(), JSON_IS_VALUE()?
 
More - we have this functionality already

ostgres=# select json_typeof('[10,20]');
┌─────────────┐
│ json_typeof │
╞═════════════╡
│ array       │
└─────────────┘
(1 row)



    
Another question - when I used FORMAT JSON clause, then I got syntax error
on DEFAULT keyword .. . Is it correct? 

Why I cannot to use together FORMAT JSON and DEFAULT clauses?
JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only 
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.

This syntax is specified in the SQL standard:

<JSON table formatted column definition> ::=
  <column name> <data type> FORMAT <JSON representation>
  [ PATH <JSON table column path specification> ]
  [ <JSON table formatted column wrapper behavior> WRAPPER ]
  [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
  [ <JSON table formatted column empty behavior> ON EMPTY ]
  [ <JSON table formatted column error behavior> ON ERROR ]

<JSON table formatted column empty behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT

<JSON table formatted column error behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT


But I also think that DEFAULT clause could be very useful in JSON_QUERY and 
formatted JSON_TABLE columns.

Note - this behave is not described in documentation.
There are references to JSON_QUERY and JSON_VALUE behavior in the definitions 
of JSON_TABLE columns, but their behavior still seems to be unclear.  This 
needs to be fixed.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov
Attached 42th version of the patches rebased onto current master.


Changes from the previous version:
 * added EXISTS PATH columns
 * added DEFAULT clause for FORMAT JSON columns
 * added implicit FORMAT JSON for columns of json[b], array and composite types


On 21.11.2019 19:51, Pavel Stehule wrote:

čt 21. 11. 2019 v 17:31 odesílatel Nikita Glukhov <[hidden email]> napsal:

On 17.11.2019 13:35, Pavel Stehule wrote:

I found:
a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not. 
I think should be useful support this clause too. 
SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:
=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$'
                   COLUMNS (
                     a bool PATH 'exists($.a)',
                     b bool PATH 'exists($.b)'
                   ));
 a | b 
---+---
 t | f
(1 row)

But this works as expected only in lax mode.  In strict mode EXISTS() returns 
Unknown that transformed into SQL NULL:

=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$' 
                   COLUMNS (
                     a bool PATH 'strict exists($.a)',
                     b bool PATH 'strict exists($.b)'
                   ));
 a | b 
---+---
 t | 
(1 row)

There is no easy way to return false without external COALESCE(),
DEFAULT false ON ERROR also does not help.  

So, I think it's worth to add EXISTS PATH clause to our implementation.


    

              
There is a question how to map boolean result to other data types. 
Now, boolean result can be used in JSON_TABLE columns of bool, int4, text, 
json[b], and other types which have CAST from bool:
SELECT * 
FROM JSON_TABLE('{"a": 1}', '$'
                COLUMNS (
                  a int PATH 'exists($.a)',
                  b text PATH 'exists($.b)'
                ));
 a |   b 
---+-------
 1 | false
(1 row)
EXISTS PATH columns were added.  Only column types having CASTS 
from boolean type are accepted.

Example:

SELECT * 
FROM JSON_TABLE(
  '{"foo": "bar"}', '$'
   COLUMNS (
     foo_exists boolean EXISTS PATH '$.foo',
     foo int EXISTS,
     err text EXISTS PATH '$ / 0' TRUE ON ERROR
   )
);

 foo_exists | foo |  err 
------------+-----+------
 t          |   1 | true   
(1 row)


b) When searched value is not scalar, then it returns null. This behave can be 
suppressed by clause FORMAT Json. I found a different behave, and maybe I found
a bug.  On MySQL this clause is by default for JSON values (what has sense).
SELECT *
 FROM
      JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
         aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY 
        )
      ) AS tt;
It returns null, although it should to return [1,2]. 
Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values. 
Otherwise an error is thrown, which can be caught by ON ERROR clause. This 
behavior is specified by the standard.

FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
standard does not have any json data types, so I think we can add implicit 
FORMAT JSON for json[b] typed-columns.  But I'm a bit afraid that different 
behavior can be standardized after introduction of json data types in SQL.

There is another bug maybe. Although there is DEFAULT clause. It returns NULL.
ON ERROR should be used if "not a scalar" error needs to be caught:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
        )
    ) AS tt;

     aj     
------------
 {"x": 333}
(1 row)


ON EMPTY catches only empty-result case (for example, non-existent path in 
lax mode):

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY 
        )
    ) AS tt;
     aj     
------------
 {"x": 333}
(1 row)

I got correct result when I used FORMAT JSON clause. 
I think it should be default behave for json and jsonb columns.
I agree that FORMAT JSON could be implicit for json[b] columns.  But I think
there could be one minor problem if we want to verify that returned value is 
scalar.

Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON PATH 'lax $.a' ERROR ON ERROR 
        )
    ) AS tt;
ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item

(This error message with the reference to implicit JSON_VALUE needs to be fixed.)


But with FORMAT JSON we need to construct complex jsonpath with a filter and 
override ON EMPTY behavior:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON FORMAT JSON 
            -- strict mode is mandatory to prevent array unwrapping
            PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
            ERROR ON EMPTY ERROR ON ERROR
        )
    ) AS tt;
ERROR:  no SQL/JSON item
please, check the behave of other databases. I think so good conformance with other RDBMS is important. More this method for checking if value is object or not looks little bit scary.

maybe we can implement some functions like JSON_IS_OBJECT(), JSON_IS_ARRAY(), JSON_IS_VALUE()?
 
More - we have this functionality already

ostgres=# select json_typeof('[10,20]');
┌─────────────┐
│ json_typeof │
╞═════════════╡
│ array       │
└─────────────┘
(1 row)

Implicit FORMAT JSON is used for columns of json[b], array and composite types now.
The behavior is similar to behavior of json_populate_record().

Example:

CREATE TYPE test_record AS (foo text[], bar int);
SELECT * 
FROM JSON_TABLE(
  '{"foo": ["bar", 123, null]}', '$'
   COLUMNS (
     js json PATH '$', 
     jsonb_arr jsonb[] PATH '$.foo', 
     text_arr text[] PATH '$.foo', 
     int_arr int[] PATH '$.foo' DEFAULT '{}' ON ERROR, 
     rec test_record PATH '$'
   )
);
             js              |      jsonb_arr       |    text_arr    | int_arr |         rec         
-----------------------------+----------------------+----------------+---------+---------------------
 {"foo": ["bar", 123, null]} | {"\"bar\"",123,NULL} | {bar,123,NULL} | {}      | ("{bar,123,NULL}",)
(1 row)

Another question - when I used FORMAT JSON clause, then I got syntax error
on DEFAULT keyword .. . Is it correct? 

Why I cannot to use together FORMAT JSON and DEFAULT clauses?
JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only 
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.

This syntax is specified in the SQL standard:

<JSON table formatted column definition> ::=
  <column name> <data type> FORMAT <JSON representation>
  [ PATH <JSON table column path specification> ]
  [ <JSON table formatted column wrapper behavior> WRAPPER ]
  [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
  [ <JSON table formatted column empty behavior> ON EMPTY ]
  [ <JSON table formatted column error behavior> ON ERROR ]

<JSON table formatted column empty behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT

<JSON table formatted column error behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT


But I also think that DEFAULT clause could be very useful in JSON_QUERY and 
formatted JSON_TABLE columns.

DEFAULT clause was enabled in JSON_QUERY() and formatted JSON_TABLE columns:

SELECT * 
FROM JSON_TABLE(
  '{"foo": "bar"}', '$'
   COLUMNS (
     baz json FORMAT JSON DEFAULT '"empty"' ON EMPTY
   )
);
   baz   
---------
 "empty"
(1 row)


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-SQL-JSON-functions-v42.patch.gz (166K) Download Attachment
0002-JSON_TABLE-v42.patch.gz (37K) Download Attachment
0003-JSON_TABLE-PLAN-DEFAULT-clause-v42.patch.gz (9K) Download Attachment
0004-JSON_TABLE-PLAN-clause-v42.patch.gz (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule
Hi

I read this patch

There are some typo in doc

name type EXISTS [ PATH json_path_specification ]

Gerenates a column and inserts a boolean item into each row of this column.


Is good to allow repeat examples from documentation - so documentation should to contains a INSERT with JSON, query and result.

JSON_TABLE is pretty complex function, probably the most complex function what I know, so I propose to divide documentation to two parts - basic advanced. The basic should to coverage the work with missing or error values (with examples), and explain what are wrappers. Advanced part should to describe work with plans. I afraid so lot of smaller examples has to be necessary. Personally I propose postpone 0003 and 0004 patches to some next releases. This is extra functionality and not well used and documented in other RDBMS (depends on your capacity) - there is problem only in well documentation - because this feature is not almost used in projects, the small differences from standard or other RDBMS can be fixed later (like we fixed XMLTABLE last year).

The documentation is good enough for initial commit - but should be significantly enhanced before release.

I did some small performance tests - and parsing json with result cca 25000 rows needs 150 ms. It is great time.

My previous objections was solved.

The patches was applied cleanly. The compilation is without any issues and warnings.
There are enough regress tests, and check-world was passed without problem.
Source code is readable, and well formatted.

I checked standard and checked conformance with other RDBMS.

I will mark this patch - JSON_TABLE implementation as ready for commiter. The documentation should be enhanced - more examples, more simple examples are necessary.

Regards

Thank you for your great, complex and hard work

It will be great feature

Pavel






út 14. 1. 2020 v 16:26 odesílatel Nikita Glukhov <[hidden email]> napsal:
Attached 42th version of the patches rebased onto current master.


Changes from the previous version:
 * added EXISTS PATH columns
 * added DEFAULT clause for FORMAT JSON columns
 * added implicit FORMAT JSON for columns of json[b], array and composite types


On 21.11.2019 19:51, Pavel Stehule wrote:

čt 21. 11. 2019 v 17:31 odesílatel Nikita Glukhov <[hidden email]> napsal:

On 17.11.2019 13:35, Pavel Stehule wrote:

I found:
a) Oracle & MySQL (Oracle) supports EXISTS clause, this implementation not. 
I think should be useful support this clause too. 
SELECT * FROM JSON_TABLE('...', '...' COLUMNS x INT EXISTS PATH ...

EXISTS PATH clause can be emulated with jsonpath EXISTS() predicate:
=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$'
                   COLUMNS (
                     a bool PATH 'exists($.a)',
                     b bool PATH 'exists($.b)'
                   ));
 a | b 
---+---
 t | f
(1 row)

But this works as expected only in lax mode.  In strict mode EXISTS() returns 
Unknown that transformed into SQL NULL:

=# SELECT * 
   FROM JSON_TABLE('{"a": 1}', '$' 
                   COLUMNS (
                     a bool PATH 'strict exists($.a)',
                     b bool PATH 'strict exists($.b)'
                   ));
 a | b 
---+---
 t | 
(1 row)

There is no easy way to return false without external COALESCE(),
DEFAULT false ON ERROR also does not help.  

So, I think it's worth to add EXISTS PATH clause to our implementation.


    

              
There is a question how to map boolean result to other data types. 
Now, boolean result can be used in JSON_TABLE columns of bool, int4, text, 
json[b], and other types which have CAST from bool:
SELECT * 
FROM JSON_TABLE('{"a": 1}', '$'
                COLUMNS (
                  a int PATH 'exists($.a)',
                  b text PATH 'exists($.b)'
                ));
 a |   b 
---+-------
 1 | false
(1 row)
EXISTS PATH columns were added.  Only column types having CASTS 
from boolean type are accepted.

Example:

SELECT * 
FROM JSON_TABLE(
  '{"foo": "bar"}', '$'
   COLUMNS (
     foo_exists boolean EXISTS PATH '$.foo',
     foo int EXISTS,
     err text EXISTS PATH '$ / 0' TRUE ON ERROR
   )
);

 foo_exists | foo |  err 
------------+-----+------
 t          |   1 | true   
(1 row)


b) When searched value is not scalar, then it returns null. This behave can be 
suppressed by clause FORMAT Json. I found a different behave, and maybe I found
a bug.  On MySQL this clause is by default for JSON values (what has sense).
SELECT *
 FROM
      JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
         aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON EMPTY 
        )
      ) AS tt;
It returns null, although it should to return [1,2]. 
Yes, regular (non-formatted) JSON_TABLE columns can accept only scalar values. 
Otherwise an error is thrown, which can be caught by ON ERROR clause. This 
behavior is specified by the standard.

FORMAT JSON is not implicitly added for json[b] columns now. The current SQL
standard does not have any json data types, so I think we can add implicit 
FORMAT JSON for json[b] typed-columns.  But I'm a bit afraid that different 
behavior can be standardized after introduction of json data types in SQL.

There is another bug maybe. Although there is DEFAULT clause. It returns NULL.
ON ERROR should be used if "not a scalar" error needs to be caught:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.a' DEFAULT '{"x": 333}' ON ERROR
        )
    ) AS tt;

     aj     
------------
 {"x": 333}
(1 row)


ON EMPTY catches only empty-result case (for example, non-existent path in 
lax mode):

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS(
            aj JSON PATH '$.foo' DEFAULT '{"x": 333}' ON EMPTY 
        )
    ) AS tt;
     aj     
------------
 {"x": 333}
(1 row)

I got correct result when I used FORMAT JSON clause. 
I think it should be default behave for json and jsonb columns.
I agree that FORMAT JSON could be implicit for json[b] columns.  But I think
there could be one minor problem if we want to verify that returned value is 
scalar.

Without FORMAT JSON this is verified by the underlying JSON_VALUE expression:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON PATH 'lax $.a' ERROR ON ERROR 
        )
    ) AS tt;
ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item

(This error message with the reference to implicit JSON_VALUE needs to be fixed.)


But with FORMAT JSON we need to construct complex jsonpath with a filter and 
override ON EMPTY behavior:

SELECT *
FROM
    JSON_TABLE(
        '[{"a":[1,2]}]',
        '$[*]'
        COLUMNS (
            aj JSON FORMAT JSON 
            -- strict mode is mandatory to prevent array unwrapping
            PATH 'strict $.a ? (@.type() != "array" && @.type() != "object")'
            ERROR ON EMPTY ERROR ON ERROR
        )
    ) AS tt;
ERROR:  no SQL/JSON item
please, check the behave of other databases. I think so good conformance with other RDBMS is important. More this method for checking if value is object or not looks little bit scary.

maybe we can implement some functions like JSON_IS_OBJECT(), JSON_IS_ARRAY(), JSON_IS_VALUE()?
 
More - we have this functionality already

ostgres=# select json_typeof('[10,20]');
┌─────────────┐
│ json_typeof │
╞═════════════╡
│ array       │
└─────────────┘
(1 row)

Implicit FORMAT JSON is used for columns of json[b], array and composite types now.
The behavior is similar to behavior of json_populate_record().

Example:

CREATE TYPE test_record AS (foo text[], bar int);
SELECT * 
FROM JSON_TABLE(
  '{"foo": ["bar", 123, null]}', '$'
   COLUMNS (
     js json PATH '$', 
     jsonb_arr jsonb[] PATH '$.foo', 
     text_arr text[] PATH '$.foo', 
     int_arr int[] PATH '$.foo' DEFAULT '{}' ON ERROR, 
     rec test_record PATH '$'
   )
);
             js              |      jsonb_arr       |    text_arr    | int_arr |         rec         
-----------------------------+----------------------+----------------+---------+---------------------
 {"foo": ["bar", 123, null]} | {"\"bar\"",123,NULL} | {bar,123,NULL} | {}      | ("{bar,123,NULL}",)
(1 row)

Another question - when I used FORMAT JSON clause, then I got syntax error
on DEFAULT keyword .. . Is it correct? 

Why I cannot to use together FORMAT JSON and DEFAULT clauses?
JSON_TABLE columns with FORMAT JSON, like JSON_QUERY, can have only 
ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT behaviors.

This syntax is specified in the SQL standard:

<JSON table formatted column definition> ::=
  <column name> <data type> FORMAT <JSON representation>
  [ PATH <JSON table column path specification> ]
  [ <JSON table formatted column wrapper behavior> WRAPPER ]
  [ <JSON table formatted column quotes behavior> QUOTES [ ON SCALAR STRING ] ]
  [ <JSON table formatted column empty behavior> ON EMPTY ]
  [ <JSON table formatted column error behavior> ON ERROR ]

<JSON table formatted column empty behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT

<JSON table formatted column error behavior> ::=
  ERROR
  | NULL
  | EMPTY ARRAY
  | EMPTY OBJECT


But I also think that DEFAULT clause could be very useful in JSON_QUERY and 
formatted JSON_TABLE columns.
DEFAULT clause was enabled in JSON_QUERY() and formatted JSON_TABLE columns:

SELECT * 
FROM JSON_TABLE(
  '{"foo": "bar"}', '$'
   COLUMNS (
     baz json FORMAT JSON DEFAULT '"empty"' ON EMPTY
   )
);
   baz   
---------
 "empty"
(1 row)


--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Pavel Stehule
Hi

This patch needs rebase

Regards

Pavel
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov
On 23.03.2020 19:24, Pavel Stehule wrote:

> This patch needs rebase


Attached 43rd version of the patches based on the latest (v47) SQL/JSON
functions patches.

Nothing significant has changed from the previous version, excluding
removed support for json type.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-SQL-JSON-functions-v43.patch.gz (99K) Download Attachment
0002-JSON_TABLE-v43.patch.gz (35K) Download Attachment
0003-JSON_TABLE-PLAN-DEFAULT-clause-v43.patch.gz (9K) Download Attachment
0004-JSON_TABLE-PLAN-clause-v43.patch.gz (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Justin Pryzby
On Mon, Mar 23, 2020 at 08:33:34PM +0300, Nikita Glukhov wrote:
> On 23.03.2020 19:24, Pavel Stehule wrote:
> > This patch needs rebase
>
> Attached 43rd version of the patches based on the latest (v47) SQL/JSON
> functions patches.

It looks like this needs to be additionally rebased - I will set cfbot to
"Waiting".

--
Justin


Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Michael Paquier-2
On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:
> It looks like this needs to be additionally rebased - I will set cfbot to
> "Waiting".

...  Something that has not happened in four weeks, so this is marked
as returned with feedback.  Please feel free to resubmit once a rebase
is done.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov


On 03.08.2020 10:55, Michael Paquier wrote:
On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:
It looks like this needs to be additionally rebased - I will set cfbot to
"Waiting".
...  Something that has not happened in four weeks, so this is marked
as returned with feedback.  Please feel free to resubmit once a rebase
is done.
--
Michael

Atatched 44th version of the pacthes rebased onto current master
(#0001 corresponds to v51 of SQL/JSON patches).

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-SQL-JSON-functions-v44.patch.gz (118K) Download Attachment
0002-JSON_TABLE-v44.patch.gz (36K) Download Attachment
0003-JSON_TABLE-PLAN-DEFAULT-clause-v44.patch.gz (9K) Download Attachment
0004-JSON_TABLE-PLAN-clause-v44.patch.gz (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Zhihong Yu
For new files introduced in the patches:

+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group

2021 is a few days ahead. It would be good to update the year range.

For transformJsonTableColumn:

+   jfexpr->op =
+       jtc->coltype == JTC_REGULAR ? IS_JSON_VALUE :
+       jtc->coltype == JTC_EXISTS ? IS_JSON_EXISTS : IS_JSON_QUERY;

Should IS_JSON_EXISTS be mentioned in the comment preceding the method ?

For JsonTableDestroyOpaque():

+   state->opaque = NULL;

Should the memory pointed to by opaque be freed ?

+   l2 = list_head(tf->coltypes);
+   l3 = list_head(tf->coltypmods);
+   l4 = list_head(tf->colvalexprs);

Maybe the ListCell pointer variables can be named corresponding to the lists they iterate so that the code is easier to understand.

+typedef enum JsonTablePlanJoinType
+{
+   JSTP_INNER = 0x01,
+   JSTP_OUTER = 0x02,
+   JSTP_CROSS = 0x04,

Since plan type enum starts with JSTP_, I think the plan join type should start with JSTPJ_ or JSTJ_. Otherwise the following would be a bit hard to read:

+   else if (plan->plan_type == JSTP_JOINED)
+   {
+       if (plan->join_type == JSTP_INNER ||
+           plan->join_type == JSTP_OUTER)

since different fields are checked in the two if statements but the prefixes don't convey that.

+      Even though the path names are not incuded into the <literal>PLAN DEFAULT</literal>

Typo: incuded -> included

+   int         nchilds = 0;

nchilds -> nchildren

+#if 0 /* XXX it' unclear from the standard whether root path name is mandatory or not */
+   if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)

Do you plan to drop the if block ?

Cheers

On Fri, Dec 25, 2020 at 12:32 PM Nikita Glukhov <[hidden email]> wrote:


On 03.08.2020 10:55, Michael Paquier wrote:
On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:
It looks like this needs to be additionally rebased - I will set cfbot to
"Waiting".
...  Something that has not happened in four weeks, so this is marked
as returned with feedback.  Please feel free to resubmit once a rebase
is done.
--
Michael

Atatched 44th version of the pacthes rebased onto current master
(#0001 corresponds to v51 of SQL/JSON patches).

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
123