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.

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.

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.