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
Thank you for review.
Attached 45th version of the patches. "SQL/JSON functions" patch corresponds to 
v52 patch set posted in the separate thread.

On 27.12.2020 01:16, Zhihong Yu wrote:
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.
Fixed.

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 ?
Added mention of EXISTS PATH column to the comment.


For JsonTableDestroyOpaque():

+   state->opaque = NULL;

Should the memory pointed to by opaque be freed ?
I think it's not necessary, because FunctionScan, caller of
JsonTableDestroyOpaque(), will free it and also all opaque's fields using
MemoryContextReset().


+   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.
Variable were renamed, also foreach() loop was refactored using forfour() macro.


+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.
Enumeration members were renames using prefix JSTPJ_.

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

Typo: incuded -> included
Fixed.

+   int         nchilds = 0;

nchilds -> nchildren
Fixed.


+#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 ?
If it becomes clear, I will drop it.
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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

Re: SQL/JSON: JSON_TABLE

David Steele
On 1/20/21 8:42 PM, Nikita Glukhov wrote:
> Thank you for review.
>
> Attached 45th version of the patches. "SQL/JSON functions" patch corresponds to
> v52 patch set posted in the separate thread.

Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log),
marked Waiting on Author.

I can see that Álvaro suggested that the patch be split up so it can be
reviewed and committed in pieces. It looks like you've done that to some
extent, but I wonder if more can be done. In particular, it looks like
that first patch could be broken up -- at lot.

Regards,
--
-David
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Andrew Dunstan

On 3/25/21 8:10 AM, David Steele wrote:

> On 1/20/21 8:42 PM, Nikita Glukhov wrote:
>> Thank you for review.
>>
>> Attached 45th version of the patches. "SQL/JSON functions" patch
>> corresponds to
>> v52 patch set posted in the separate thread.
>
> Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log),
> marked Waiting on Author.
>
> I can see that Álvaro suggested that the patch be split up so it can
> be reviewed and committed in pieces. It looks like you've done that to
> some extent, but I wonder if more can be done. In particular, it looks
> like that first patch could be broken up -- at lot.
>
>
I've rebased this. Note that the large first patch is just the
accumulated patches from the 'SQL/JSON functions' thread, and should be
reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
there are no changes at all in those patches from the previous set other
than a little patch fuzz. The only substantial changes are in patch 1,
which had bitrotted. However, I'm posting a new series to keep the
numbering in sync.


If the cfbot is happy I will set back to 'Needs review'


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com


0001-SQL-JSON-functions-v46.patch.gz (99K) Download Attachment
0002-JSON_TABLE-v46.patch.gz (25K) Download Attachment
0003-JSON_TABLE-PLAN-DEFAULT-clause-v46.patch.gz (8K) Download Attachment
0004-JSON_TABLE-PLAN-clause-v46.patch.gz (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Erik Rijkers
> On 2021.03.26. 21:28 Andrew Dunstan <[hidden email]> wrote:
> On 3/25/21 8:10 AM, David Steele wrote:
> > On 1/20/21 8:42 PM, Nikita Glukhov wrote:
> >> Thank you for review.
> >>
> >> Attached 45th version of the patches. "SQL/JSON functions" patch
> >> corresponds to
> >> v52 patch set posted in the separate thread.
> >
> > Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log),
> > marked Waiting on Author.
> >
> > I can see that Álvaro suggested that the patch be split up so it can
> > be reviewed and committed in pieces. It looks like you've done that to
> > some extent, but I wonder if more can be done. In particular, it looks
> > like that first patch could be broken up -- at lot.
> >
> >
>
> I've rebased this. Note that the large first patch is just the
> accumulated patches from the 'SQL/JSON functions' thread, and should be
> reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
> there are no changes at all in those patches from the previous set other
> than a little patch fuzz. The only substantial changes are in patch 1,
> which had bitrotted. However, I'm posting a new series to keep the
> numbering in sync.
>
> If the cfbot is happy I will set back to 'Needs review'

> 0001-SQL-JSON-functions-v46.patch
> 0002-JSON_TABLE-v46.patch
> 0003-JSON_TABLE-PLAN-DEFAULT-clause-v46.patch
> 0004-JSON_TABLE-PLAN-clause-v46.patch


Hi,

The four v46 patches apply fine, but on compile I get (debian/gcc):

make --quiet -j 4
make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'.  Stop.
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [parser-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
common.mk:39: recipe for target 'parser-recursive' failed
Makefile:42: recipe for target 'all-backend-recurse' failed
GNUmakefile:11: recipe for target 'all-src-recurse' failed


Erik


Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Andrew Dunstan

On 3/26/21 4:48 PM, Erik Rijkers wrote:

>> On 2021.03.26. 21:28 Andrew Dunstan <[hidden email]> wrote:
>> On 3/25/21 8:10 AM, David Steele wrote:
>>> On 1/20/21 8:42 PM, Nikita Glukhov wrote:
>>>> Thank you for review.
>>>>
>>>> Attached 45th version of the patches. "SQL/JSON functions" patch
>>>> corresponds to
>>>> v52 patch set posted in the separate thread.
>>> Another rebase needed (http://cfbot.cputube.org/patch_32_2902.log),
>>> marked Waiting on Author.
>>>
>>> I can see that Álvaro suggested that the patch be split up so it can
>>> be reviewed and committed in pieces. It looks like you've done that to
>>> some extent, but I wonder if more can be done. In particular, it looks
>>> like that first patch could be broken up -- at lot.
>>>
>>>
>> I've rebased this. Note that the large first patch is just the
>> accumulated patches from the 'SQL/JSON functions' thread, and should be
>> reviewed there. Only patches 2 thru 4 should be reviewed here. In fact
>> there are no changes at all in those patches from the previous set other
>> than a little patch fuzz. The only substantial changes are in patch 1,
>> which had bitrotted. However, I'm posting a new series to keep the
>> numbering in sync.
>>
>> If the cfbot is happy I will set back to 'Needs review'
>> 0001-SQL-JSON-functions-v46.patch
>> 0002-JSON_TABLE-v46.patch
>> 0003-JSON_TABLE-PLAN-DEFAULT-clause-v46.patch
>> 0004-JSON_TABLE-PLAN-clause-v46.patch
>
> Hi,
>
> The four v46 patches apply fine, but on compile I get (debian/gcc):
>
> make --quiet -j 4
> make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'.  Stop.
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [parser-recursive] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [all-backend-recurse] Error 2
> make: *** [all-src-recurse] Error 2
> common.mk:39: recipe for target 'parser-recursive' failed
> Makefile:42: recipe for target 'all-backend-recurse' failed
> GNUmakefile:11: recipe for target 'all-src-recurse' failed
>


Yeah, I messed up :-( Forgot to git-add some files.


will repost soon.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com



Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov

Attached 47th version of the patches.

On 26.03.2021 23:58, Andrew Dunstan wrote:
On 3/26/21 4:48 PM, Erik Rijkers wrote:

Hi,

The four v46 patches apply fine, but on compile I get (debian/gcc):

make --quiet -j 4
make[3]: *** No rule to make target 'parse_jsontable.o', needed by 'objfiles.txt'.  Stop.
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [parser-recursive] Error 2
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [all-backend-recurse] Error 2
make: *** [all-src-recurse] Error 2
common.mk:39: recipe for target 'parser-recursive' failed
Makefile:42: recipe for target 'all-backend-recurse' failed
GNUmakefile:11: recipe for target 'all-src-recurse' failed
Yeah, I messed up :-( Forgot to git-add some files.


will repost soon.
I have added forgotten files and fixed the first patch.

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

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

Re: SQL/JSON: JSON_TABLE

Erik Rijkers
> On 2021.03.27. 02:12 Nikita Glukhov <[hidden email]> wrote:
>
> Attached 47th version of the patches.
>
[..]
>
> I have added forgotten files and fixed the first patch.
>
> [0001-SQL-JSON-functions-v47.patch]
> [0002-JSON_TABLE-v47.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
> [0004-JSON_TABLE-PLAN-clause-v47.patch]

Hi,

Apply, build all fine.  It also works quite well, and according to specification, as far as I can tell.

But today I ran into:

ERROR:  function ExecEvalJson not in llvmjit_types.c

I think that it is caused by:

set enable_bitmapscan = off;

(I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).


This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):

select jt1.*
from myjsonfile100k as t(js, id)
  , json_table(
      t.js
   , '$' columns (
        "lastname"   text    path  '$. "lastname"     '
      , "firstname"  text    path  '$. "firstname"    '
      , "date"       text    path  '$. "date"         '
      , "city"       text    path  '$. "city"         '
      , "country"    text    path  '$. "country"      '
      , "name 0(1)"  text    path  '$. "array"[0]     '
      , "name 4(5)"  text    path  '$. "array"[4]     '
      , "names"      text[]  path  '$. "array"        '
      , "randfloat"  float   path  '$. "random float" '
    )
) as jt1
where  js @> ('[ { "city": "Santiago de Cuba" } ]')
   and js[0]->>'firstname' = 'Gilda'
;
ERROR:  function ExecEvalJson not in llvmjit_types.c

That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one understands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5 GB).


Erik Rijkers


Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Nikita Glukhov

On 30.03.2021 19:56, Erik Rijkers wrote:

On 2021.03.27. 02:12 Nikita Glukhov [hidden email] wrote:

Attached 47th version of the patches.
Hi,

Apply, build all fine.  It also works quite well, and according to specification, as far as I can tell.

But today I ran into:

ERROR:  function ExecEvalJson not in llvmjit_types.c

I think that it is caused by:

set enable_bitmapscan = off;

(I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).


This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):

select jt1.* 
from myjsonfile100k as t(js, id) 
  , json_table(
      t.js
   , '$' columns (
        "lastname"   text    path  '$. "lastname"     '
      , "firstname"  text    path  '$. "firstname"    '
      , "date"       text    path  '$. "date"         '
      , "city"       text    path  '$. "city"         '
      , "country"    text    path  '$. "country"      '
      , "name 0(1)"  text    path  '$. "array"[0]     '
      , "name 4(5)"  text    path  '$. "array"[4]     '
      , "names"      text[]  path  '$. "array"        '
      , "randfloat"  float   path  '$. "random float" '
    )
) as jt1
where  js @> ('[ { "city": "Santiago de Cuba" } ]')
   and js[0]->>'firstname' = 'Gilda'
;
ERROR:  function ExecEvalJson not in llvmjit_types.c

That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one understands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5 GB).
Thank you for testing. 


I think you can try to add 3 missing functions references to the end of 
src/backend/jit/llvm/llvmjit_types.c:
 void       *referenced_functions[] =
 {
     ...
     ExecEvalXmlExpr,
+    ExecEvalJsonConstructor,
+    ExecEvalIsJsonPredicate,
+    ExecEvalJson,
     MakeExpandedObjectReadOnlyInternal,
     ...
 };


If this fixes problem, I will add this to the new version of the patches.


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

Re: SQL/JSON: JSON_TABLE

Erik Rijkers

> On 2021.03.30. 22:25 Nikita Glukhov <[hidden email]> wrote:
>
>  
> On 30.03.2021 19:56, Erik Rijkers wrote:
>
> >> On 2021.03.27. 02:12 Nikita Glukhov <[hidden email]> wrote:
> >>
> >> Attached 47th version of the patches.
> > Hi,
> >
> > Apply, build all fine.  It also works quite well, and according to specification, as far as I can tell.
> >
> > But today I ran into:
> >
> > ERROR:  function ExecEvalJson not in llvmjit_types.c
> >
> > I think that it is caused by:
> >
> > set enable_bitmapscan = off;
> >
> > (I installed llvm a few days ago. llvm-3.9-dev on this debian stretch).
> >
> >
> > This is the test sql I concocted, which runs fine with enable_bitmapscan on (the default):
> >
> > select jt1.*
> > from myjsonfile100k as t(js, id)
> >    , json_table(
> >        t.js
> >     , '$' columns (
> >          "lastname"   text    path  '$. "lastname"     '
> >        , "firstname"  text    path  '$. "firstname"    '
> >        , "date"       text    path  '$. "date"         '
> >        , "city"       text    path  '$. "city"         '
> >        , "country"    text    path  '$. "country"      '
> >        , "name 0(1)"  text    path  '$. "array"[0]     '
> >        , "name 4(5)"  text    path  '$. "array"[4]     '
> >        , "names"      text[]  path  '$. "array"        '
> >        , "randfloat"  float   path  '$. "random float" '
> >      )
> > ) as jt1
> > where  js @> ('[ { "city": "Santiago de Cuba" } ]')
> >     and js[0]->>'firstname' = 'Gilda'
> > ;
> > ERROR:  function ExecEvalJson not in llvmjit_types.c
> >
> > That statement only errors out if the table is large enough. I have no time now to make a sample table but if no-one understands the problem off-hand, I'll try to construct such a table later this week (the one I'm using is large, 1.5 GB).
>
> Thank you for testing.
>
>
> I think you can try to add 3 missing functions references to the end of
> src/backend/jit/llvm/llvmjit_types.c:
>
>   void       *referenced_functions[] =
>   {
>       ...
>       ExecEvalXmlExpr,
> +    ExecEvalJsonConstructor,
> +    ExecEvalIsJsonPredicate,
> +    ExecEvalJson,
>       MakeExpandedObjectReadOnlyInternal,
>       ...
>   };
>
>
> If this fixes problem, I will add this to the new version of the patches.

It does almost fix it, but in the above is a typo:
+  ExecEvalIsJsonPredicate     should to be changed to:
+  ExecEvalJsonIsPredicate.

With that change the problem vanishes.

Thanks!

Erik Rijkers








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


Reply | Threaded
Open this post in threaded view
|

Re: SQL/JSON: JSON_TABLE

Erik Rijkers
In reply to this post by Nikita Glukhov
> On 2021.03.27. 02:12 Nikita Glukhov <[hidden email]> wrote:
> Attached 47th version of the patches.

We're past feature freeze for 14 and alas, JSON_TABLE has not made it.

I have tested quite a bit with it and because I didn't find any trouble with functionality or speed, I wanted to at least mention that here once.

I looked at v47, these files
> [0001-SQL-JSON-functions-v47.patch]
> [0002-JSON_TABLE-v47.patch]
> [0003-JSON_TABLE-PLAN-DEFAULT-clause-v47.patch]
> [0004-JSON_TABLE-PLAN-clause-v47.patch]
> [manual_addition_fixed.patch]  # for this see [1], [2]

   (v47 doesn't apply anymore, as cfbot shows, but instances can still be built on top of 6131ffc43ff from 30 march 2021)

I hope it will fare better next round, version 15.

Thanks,

Erik Rijkers

[1] https://www.postgresql.org/message-id/69eefc5a-cabc-8dd3-c689-93da038c0d6a%40postgrespro.ru
[2] https://www.postgresql.org/message-id/19181987.22943.1617141503618%40webmailclassic.xs4all.nl


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


123