Re: remaining sql/json patches

Hi
v33-0007-SQL-JSON-query-functions.patch, commit message:
This introduces the SQL/JSON functions for querying JSON data using
jsonpath expressions. The functions are:

should it be "These functions are"

+ <para>
+ Returns true if the SQL/JSON <replaceable>path_expression</replaceable>
+ applied to the <replaceable>context_item</replaceable> using the
+ <replaceable>value</replaceable>s yields any items.
+ The <literal>ON ERROR</literal> clause specifies what is returned if
+ an error occurs; the default is to return <literal>FALSE</literal>.
+ Note that if the <replaceable>path_expression</replaceable>
+ is <literal>strict</literal>, an error is generated if it
yields no items.
+ </para>

I think the following description is more accurate.
+ Note that if the <replaceable>path_expression</replaceable>
+ is <literal>strict</literal> and the <literal>ON
ERROR</literal> clause is <literal> ERROR</literal>,
+ an error is generated if it yields no items.
+ </para>

+/*
+ * transformJsonTable -
+ * Transform a raw JsonTable into TableFunc.
+ *
+ * Transform the document-generating expression, the row-generating expression,
+ * the column-generating expressions, and the default value expressions.
+ */
+ParseNamespaceItem *
+transformJsonTable(ParseState *pstate, JsonTable *jt)
+{
+ JsonTableParseContext cxt;
+ TableFunc *tf = makeNode(TableFunc);
+ JsonFuncExpr *jfe = makeNode(JsonFuncExpr);
+ JsonExpr *je;
+ JsonTablePlan *plan = jt->plan;
+ char *rootPathName = jt->pathname;
+ char *rootPath;
+ bool is_lateral;
+
+ if (jt->on_empty)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ON EMPTY not allowed in JSON_TABLE"),
+ parser_errposition(pstate,
+ exprLocation((Node *) jt->on_empty))));

This error may be slightly misleading?
you can add ON EMPTY inside the COLUMNS part, like the following:
SELECT * FROM (VALUES ('1'), ('"1"')) vals(js) LEFT OUTER JOIN
JSON_TABLE(vals.js::jsonb, '$' COLUMNS (a int PATH '$' default 1 ON
empty)) jt ON true;

+ <para>
+ Each <literal>NESTED PATH</literal> clause can generate one or more
+ columns. Columns produced by <literal>NESTED PATH</literal>s at the
+ same level are considered to be <firstterm>siblings</firstterm>,
+ while a column produced by a <literal>NESTED PATH</literal> is
+ considered to be a child of the column produced by a
+ <literal>NESTED PATH</literal> or row expression at a higher level.
+ Sibling columns are always joined first. Once they are processed,
+ the resulting rows are joined to the parent row.
+ </para>
Does changing to the following make sense?
+ considered to be a <firstterm>child</firstterm> of the column produced by a
+ the resulting rows are joined to the <firstterm>parent</firstterm> row.

seems like `format json_representation`, not listed in the
documentation, but json_representation is "Parameters", do we need
add a section to explain it?
even though I think currently we can only do `FORMAT JSON`.

SELECT * FROM JSON_TABLE(jsonb '123', '$' COLUMNS (item int PATH '$'
empty on empty)) bar;
ERROR: cannot cast jsonb array to type integer
The error is the same as the output of the following:
SELECT * FROM JSON_TABLE(jsonb '123', '$' COLUMNS (item int PATH '$'
empty array on empty )) bar;
but these two are different things?

+ /* FALLTHROUGH */
+ case JTC_EXISTS:
+ case JTC_FORMATTED:
+ {
+ Node *je;
+ CaseTestExpr *param = makeNode(CaseTestExpr);
+
+ param->collation = InvalidOid;
+ param->typeId = cxt->contextItemTypid;
+ param->typeMod = -1;
+
+ if (rawc->wrapper != JSW_NONE &&
+ rawc->quotes != JS_QUOTES_UNSPEC)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot use WITH WRAPPER clause for formatted colunmns"
+ " without also specifying OMIT/KEEP QUOTES"),
+ parser_errposition(pstate, rawc->location)));

typo, should be "formatted columns".
I suspect people will be confused with the meaning of "formatted column".
maybe we can replace this part:"cannot use WITH WRAPPER clause for
formatted column"
to
"SQL/JSON WITH WRAPPER behavior must not be specified when FORMAT
clause is used"

SELECT * FROM JSON_TABLE(jsonb '"world"', '$' COLUMNS (item text
FORMAT JSON PATH '$' with wrapper KEEP QUOTES));
ERROR: cannot use WITH WRAPPER clause for formatted colunmns without
also specifying OMIT/KEEP QUOTES
LINE 1: ...T * FROM JSON_TABLE(jsonb '"world"', '$' COLUMNS (item text ...
^
this error is misleading, since now I am using WITH WRAPPER clause for
formatted columns and specified KEEP QUOTES.

in parse_expr.c, we have errmsg("SQL/JSON QUOTES behavior must not be
specified when WITH WRAPPER is used").

+/*
+ * Fetch next row from a cross/union joined scan.
+ *
+ * Returns false at the end of a scan, true otherwise.
+ */
+static bool
+JsonTablePlanNextRow(JsonTablePlanState * state)
+{
+ JsonTableJoinState *join;
+
+ if (state->type == JSON_TABLE_SCAN_STATE)
+ return JsonTableScanNextRow((JsonTableScanState *) state);
+
+ join = (JsonTableJoinState *) state;
+ if (join->advanceRight)
+ {
+ /* fetch next inner row */
+ if (JsonTablePlanNextRow(join->right))
+ return true;
+
+ /* inner rows are exhausted */
+ if (join->cross)
+ join->advanceRight = false; /* next outer row */
+ else
+ return false; /* end of scan */
+ }
+
+ while (!join->advanceRight)
+ {
+ /* fetch next outer row */
+ bool left = JsonTablePlanNextRow(join->left);

+ bool left = JsonTablePlanNextRow(join->left);
JsonTablePlanNextRow function comment says "Returns false at the end
of a scan, true otherwise.",
so bool variable name as "left" seems not so good?

It might help others understand the whole code by adding some comments on
struct JsonTableScanState and struct JsonTableJoinState.
since json_table patch is quite recursive, IMHO.

I did some minor refactoring in parse_expr.c, since some code like
transformJsonExprCommon is duplicated.