Re: SQL/JSON: functions - Mailing list pgsql-hackers
| From | Pavel Stehule |
|---|---|
| Subject | Re: SQL/JSON: functions |
| Date | |
| Msg-id | CAFj8pRB=hPYBBsYE9nqqOnd+zHGJXXzvjw4w+KeX0UXYTdWHDg@mail.gmail.com Whole thread Raw |
| In response to | Re: SQL/JSON: functions (Nikita Glukhov <n.gluhov@postgrespro.ru>) |
| Responses |
Re: SQL/JSON: functions
|
| List | pgsql-hackers |
ú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.
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);
+<->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); }
+<-><--><-->|
+*/
+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
pgsql-hackers by date: