public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikita Glukhov <[email protected]>
To: Pavel Stehule <[email protected]>
To: PostgreSQL-development <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Andrew Alsup <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Dmitry Dolgov <[email protected]>
Cc: Oleg Bartunov <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Simon Riggs <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Subject: Re: SQL/JSON: functions
Date: Sat, 21 Mar 2020 13:07:35 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAFj8pRBVgH6zWLsBCgXX7Vqbw6UOsZkrfr2qmoW98t0gTAxk9A@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CAFj8pRBpkMC6+iSVA_k_ByAjeA2PEgixAsSiuAWy+uYp67ucbA@mail.gmail.com>
<[email protected]>
<CAFj8pRCnzO2cnHi5ebXciV=tuGVvAQOW9uPU+DQV1GkL31R=-g@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<CAFj8pRCj3fnGRe8Jj6QOUR_ohs=0DdYQ_mFSopJw=jgdfwgWKQ@mail.gmail.com>
<[email protected]>
<CAFj8pRBLk2Nhh692v1Zg0jgOdppreurVDBQt-_pj+CDEHE+HxA@mail.gmail.com>
<[email protected]>
<CAFj8pRB=hPYBBsYE9nqqOnd+zHGJXXzvjw4w+KeX0UXYTdWHDg@mail.gmail.com>
<[email protected]>
<CAFj8pRBVgH6zWLsBCgXX7Vqbw6UOsZkrfr2qmoW98t0gTAxk9A@mail.gmail.com>
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
> <[email protected] <mailto:[email protected]>> 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
>> <[email protected] <mailto:[email protected]>> 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
Attachments:
[application/gzip] 0001-Add-common-SQL-JSON-clauses-v46.patch.gz (8.0K, ../[email protected]/3-0001-Add-common-SQL-JSON-clauses-v46.patch.gz)
download
[application/gzip] 0002-SQL-JSON-constructors-v46.patch.gz (31.3K, ../[email protected]/4-0002-SQL-JSON-constructors-v46.patch.gz)
download
[application/gzip] 0003-SQL-JSON-IS-JSON-predicate-v46.patch.gz (12.5K, ../[email protected]/5-0003-SQL-JSON-IS-JSON-predicate-v46.patch.gz)
download
[application/gzip] 0004-SQL-JSON-query-functions-v46.patch.gz (36.6K, ../[email protected]/6-0004-SQL-JSON-query-functions-v46.patch.gz)
download
view thread (136+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: SQL/JSON: functions
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox