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