Re: SQL/JSON: functions

út 17. 3. 2020 v 1:55 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)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.

>
>
> 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.

+<->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)

??

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.

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); }
+<-><--><-->|
+*/

> 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" ?

Regards

Pavel

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