Re: SQL/JSON: functions

Hi, Zhihong. Thank your for your review. Attached 52nd version of the
patches rebased onto master and fixed as you suggested.

On 26.12.2020 04:19, Zhihong Yu 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 ?
>
Fixed.

> For getJsonEncodingConst(), should the method error out for the
> default case of switch (encoding) ?

Added default case with elog(ERROR).

>
> 0002-SQL-JSON-constructors-v51.patch :
>
> +                   Assert(!OidIsValid(collation)); /* result is
> always an json[b] type */
> an json -> a json

Fixed.

>
> +           /* XXX TEXTOID is default by standard */
> +           returning->typid = JSONOID;
>
> Comment doesn't seem to match the assignment.

Comment corrected.

> For json_object_agg_transfn :
>
> +       if (out->len > 2)
> +           appendStringInfoString(out, ", ");
>
> Why length needs to be at least 3 (instead of 2) ?

2 is a length of starting string "{ ".  len > 2 means that we have already
outputted some fields and we need to output comma delimiter.

On 26.12.2020 22:12, Zhihong Yu wrote:
> 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.

"If" statement was refactored as you suggest.

> 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 ?

Now *resnull is set to false in case of error.

> 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.

Assignment was removed.

> For raw_expression_tree_walker :
>
> +               if (walker(jfe->on_empty, context))
> +                   return true;
>
> Should the if condition include jfe->on_empty prior to walking ?
>
Yes, jfe->on_empty like jfe->on_error can be NULL, and NULL check here is
a walker's responsibility. But in expression_tree_walker() there is a check
for jfe->on_empty, because only on_empty (not jfe->on_error) can be NULL, and
we are calling walker() on jfe->on_empty->default_expr, not on jfe->on_empty.

>
> 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.

Code was refactored as you suggested.

> For JsonPathDatatypeStatus,
>
> +   jpdsDateTime,               /* unknown datetime type */
>
> Should the enum be named jpdsUnknownDateTime so that its meaning is
> clear to people reading the code ?

jpdsDateTime was renamed to jpdsUnknownDateTime.

>
> 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",
> ...
> };
>
"Switch" statement was replaced with array lookup.

--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company