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 <z...@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.glu...@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 >> >>