Re: SQL/JSON: functions
От | Zhihong Yu |
---|---|
Тема | Re: SQL/JSON: functions |
Дата | |
Msg-id | CALNJ-vSmJbaoUm7nuyV65UdeYTee2UxqG49CZ7CjR_rEB5ZdJg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: SQL/JSON: functions (Zhihong Yu <zyu@yugabyte.com>) |
Ответы |
Re: SQL/JSON: functions
(Nikita Glukhov <n.gluhov@postgrespro.ru>)
|
Список | pgsql-hackers |
Hi,
For ExecEvalJsonExprSubtrans(), if you check !subtrans first,
+ /* No need to use subtransactions. */
+ return func(op, econtext, res, resnull, p, error);
+ return func(op, econtext, res, resnull, p, error);
The return statement would allow omitting the else keyword and left-indent the code in the current if block.
For ExecEvalJsonExpr()
+ *resnull = !DatumGetPointer(res);
+ if (error && *error)
+ return (Datum) 0;
+ if (error && *error)
+ return (Datum) 0;
Suppose *resnull is false and *error is true, 0 would be returned with *resnull as false. Should the *resnull be consistent with the actual return value ?
For ExecEvalJson() :
+ Assert(*op->resnull);
+ *op->resnull = true;
+ *op->resnull = true;
I am not sure of the purpose for the assignment since *op->resnull should be true by the assertion.
For raw_expression_tree_walker :
+ if (walker(jfe->on_empty, context))
+ return true;
+ return true;
Should the if condition include jfe->on_empty prior to walking ?
nit: for contain_mutable_functions_walker, if !IsA(jexpr->path_spec, Const) is checked first (and return), the current if block can be left indented.
For JsonPathDatatypeStatus,
+ jpdsDateTime, /* unknown datetime type */
Should the enum be named jpdsUnknownDateTime so that its meaning is clear to people reading the code ?
For get_json_behavior(), I wonder if mapping from behavior->btype to the string form would shorten the body of switch statement.
e.g.
char* map[] = {
" NULL",
" ERROR",
" EMPTY",
...
};
Cheers
On Fri, Dec 25, 2020 at 5:19 PM Zhihong Yu <zyu@yugabyte.com> wrote:
For 0001-Common-SQL-JSON-clauses-v51.patch :+ /* | implementation_defined_JSON_representation_option (BSON, AVRO etc) */I don't find implementation_defined_JSON_representation_option in the patchset. Maybe rephrase the above as a comment without implementation_defined_JSON_representation_option ?For getJsonEncodingConst(), should the method error out for the default case of switch (encoding) ?0002-SQL-JSON-constructors-v51.patch :+ Assert(!OidIsValid(collation)); /* result is always an json[b] type */an json -> a json+ /* XXX TEXTOID is default by standard */
+ returning->typid = JSONOID;Comment doesn't seem to match the assignment.For json_object_agg_transfn :+ if (out->len > 2)
+ appendStringInfoString(out, ", ");Why length needs to be at least 3 (instead of 2) ?CheersOn Fri, Dec 25, 2020 at 12:26 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:On 17.09.2020 08:41, Michael Paquier wrote:
On Sat, Jul 18, 2020 at 09:24:11AM -0400, Andrew Dunstan wrote:I think patches 5 and 6 need to be submitted to the next commitfest, This is far too much scope creep to be snuck in under the current CF item. I'll look at patches 1-4.Even with that, the patch set has been waiting on author for the last six weeks, so I am marking it as RwF for now. Please feel free to resubmit.Attached 51st version of the patches rebased onto current master. There were some shift/reduce conflicts in SQL grammar that have appeared after "expr AS keyword" refactoring in 06a7c3154f. I'm not sure if I resolved them correctly. JSON TEXT pseudotype, introduced in #0006, caused a lot of grammar conflicts, so it was replaced with simple explicit pg_catalog.json. Also new CoercionForm COERCE_SQL_SYNTAX was introduced, and this reminds custom function formats that I have used in earlier version of the patches for deparsing of SQL/JSON constructor expressions that were based on raw json[b] function calls. These custom function formats were replaced in v43 with dedicated executor nodes for SQL/JSON constructors. So, I'm not sure is it worth to try to replace back nodes with new COERCE_SQL_SYNTAX. -- Nikita Glukhov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
В списке pgsql-hackers по дате отправления:
Следующее
От: Andres FreundДата:
Сообщение: Re: Better client reporting for "immediate stop" shutdowns