Re: SQL/JSON: functions

Hi,
For ExecEvalJsonExprSubtrans(), if you check !subtrans first,

+       /* No need to use subtransactions. */
+       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;

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;

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;

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) ?
>
> Cheers
>
> On 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
>>
>>