Re: SQL/JSON: functions

Attached 46th version of the patches.

On 20.03.2020 22:34, Pavel Stehule wrote:

čt 19. 3. 2020 v 23:57 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

Attached 45th version of the patches.

Nodes JsonFormat, JsonReturning, JsonPassing, JsonBehavior were fixed.

On 17.03.2020 21:35, Pavel Stehule wrote:

út 17. 3. 2020 v 1:55 odesílatel Nikita Glukhov <n.gluhov@postgrespro.ru> napsal:

Attached 44th version of the patches.

On 12.03.2020 16:41, Pavel Stehule wrote:

On 12.03.2020 00:09 Nikita Glukhov wrote:

Attached 43rd version of the patches.

The previous patch #4 ("Add function formats") was removed.
Instead, introduced new executor node JsonCtorExpr which is used to wrap
SQL/JSON constructor function calls (FuncExpr, Aggref, WindowFunc).

Also, JsonIsPredicate node began to be used as executor node.
This helped to remove unnecessary json[b]_is_valid() user functions.

It looks very well - all tests passed, code looks well.

Now, when there are special nodes, then the introduction of COERCE_INTERNAL_CAST is not necessary, and it is only my one and last objection again this patch's set.

I have removed patch #3 with COERCE_INTERNAL_CAST too.

Coercions in JsonValueExpr in JsonCtorExpr, which were previously hidden with
COERCE_INTERNAL_CAST and which were outputted as RETURNING or FORMAT JSON
clauses, now moved into separate expressions.

I am looking on the code, and although the code is correct it doesn't look well (consistently with other node types).

I think so JsonFormat and JsonReturning should be node types, not just structs. If these types will be nodes, then you can simplify _readJsonExpr and all node operations on this node.

 

JsonFormat and JsonReturning was transformed into nodes, and not included 
directly into other nodes now.

User functions json[b]_build_object_ext() and json[b]_build_array_ext() also 
can be easily removed.   But it seems harder to remove new aggregate functions 
json[b]_objectagg() and json[b]_agg_strict(), because they can't be called 
directly from JsonCtorExpr node.

I don't see reasons for another reduction now. Can be great if you can finalize work what you plan for pg13.

I have removed json[b]_build_object_ext() and json[b]_build_array_ext().

But json[b]_objectagg() and json[b]_agg_strict() are still present.
It seems that removing them requires majors refactoring of the execution
of Aggref and WindowFunc nodes.

+<->READ_ENUM_FIELD(on_error.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_error.default_expr);
+<->READ_ENUM_FIELD(on_empty.btype, JsonBehaviorType);
+<->READ_NODE_FIELD(on_empty.default_expr);

JsonBehavior is node type. Then why you don't write just

READ_NODE_FIELD(on_error);

READ_NODE_FIELD(on_empty)

??

JsonBehavior now used in JsonExpr as a pointer to node.

And maybe the code can be better if you don't use JsonPassing struct (or use it only inside gram.y) and pass just List *names, List *values.

Nodes should to contains another nodes or scalar types. Using structs (that are not nodes)  inside doesn't look consistently.

JsonPassing was replaced with two Lists.

I found some not finished code in 0003 patch

+
+json_name_and_value:
+/* TODO
+<-><--><-->KEY c_expr VALUE_P json_value_expr %prec POSTFIXOP
+<-><--><--><-->{ $$ = makeJsonKeyValue($2, $4); }
+<-><--><-->|
+*/

This is unsupported variant of standard syntax, because it seems to lead 
to unresolvable conflicts.  The only supports syntax is:

JSON_OBJECT(key : value)
JSON_OBJECT(key VALUE value)

ok. So please change comment from ToDo to this explanation. Maybe note in doc (unimplemented features can be good idea)

Some these unresolvable conflicts are solved with extra parenthesis in Postgres.

 

The support for json type in jsonpath also seems to be immature, so I will try 
to remove it in the next iteration.

What do you think? This patch is little bit off topic, so if you don't need it, then can be removed. Is there some dependency for "jsontable" ?

There is a dependency in SQL/JSON query functions executor, because executor 
uses new structure JsonItem instead of plain JsonbValue.  I will try to
preserve refactoring with JsonItem introduction, but remove json support.
If it will be still unacceptable, I will try to completely remove patch #1.

I have completely removed patch #1. It turned out to be not so difficult.

I have much better feeling from version 45 (now it looks like Postgres C :)). Still there are some small issues.

1. commented code

+json_encoding:
+<-><--><-->name<--><--><--><--><--><--><--><--><-->{ $$ = makeJsonEncoding($1); }
+<->/*
+<-><--><-->| UTF8<><--><--><--><--><--><--><--><-->{ $$ = JS_ENC_UTF8; }
+<-><--><-->| UTF16><--><--><--><--><--><--><--><-->{ $$ = JS_ENC_UTF16; }
+<-><--><-->| UTF32 <--><--><--><--><--><--><--><-->{ $$ = JS_ENC_UTF32; }
+<->*/
+<-><-->;

Fixed.

2. sometimes useless empty rows

                              silent boolean DEFAULT false)
+RETURNS text
+LANGUAGE INTERNAL
+STRICT IMMUTABLE PARALLEL SAFE
+AS 'jsonb_path_query_first_text';
+
+
+

+<-><-->if (!coerced)
+<-><-->{
+
+<-><--><-->/* If coercion failed, use to_json()/to_jsonb() functions. */

Fixed.

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