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 <mailto: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 <mailto: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