Re: SQL/JSON: functions

čt 19. 3. 2020 v 23:57 odesílatel Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)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(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.
>
>
> 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.
>
>
> +<->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 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; }
+<->*/
+<-><-->;

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

All tests passed
build without any problem

looking for next update

Regards

Pavel

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