Subject: Re: remaining sql/json patches

From: jian he <[email-masked]>
Date: Fri, 22 Dec 2023 21:01:05 +0800

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.

---

From: jian he <[email-masked]>
Date: Wed, 3 Jan 2024 18:50:26 +0800

> formatted columns and specified KEEP QUOTES.

Hi. still based on v33.
JSON_TABLE:
I also refactor parse_jsontable.c error reporting, now the error
message will be consistent with json_query.
now you can specify wrapper freely as long as you don't specify
wrapper and quote at the same time.
overall, json_table behavior is more consistent with json_query and json_value.
I also added some tests.

+void
+ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext)
+{
+ JsonCoercion *coercion = op->d.jsonexpr_coercion.coercion;
+ ErrorSaveContext *escontext = op->d.jsonexpr_coercion.escontext;
+ Datum res = *op->resvalue;
+ bool resnull = *op->resnull;
+
+ if (coercion->via_populate)
+ {
+ void *cache = op->d.jsonexpr_coercion.json_populate_type_cache;
+
+ *op->resvalue = json_populate_type(res, JSONBOID,
+   coercion->targettype,
+   coercion->targettypmod,
+   &cache,
+   econtext->ecxt_per_query_memory,
+   op->resnull, (Node *) escontext);
+ }
+ else if (coercion->via_io)
+ {
+ FmgrInfo   *input_finfo = op->d.jsonexpr_coercion.input_finfo;
+ Oid typioparam = op->d.jsonexpr_coercion.typioparam;
+ char   *val_string = resnull ? NULL :
+ JsonbUnquote(DatumGetJsonbP(res));
+
+ (void) InputFunctionCallSafe(input_finfo, val_string, typioparam,
+ coercion->targettypmod,
+ (Node *) escontext,
+ op->resvalue);
+ }
via_populate, via_io should be mutually exclusive.
your patch, in some cases, both (coercion->via_io) and
(coercion->via_populate) are true.
(we can use elog find out).
I refactor coerceJsonFuncExprOutput, so now it will be mutually exclusive.
I also add asserts to it.

By default, json_query keeps quotes, json_value omit quotes.
However, json_table will be transformed to json_value or json_query
based on certain criteria,
that means we need to explicitly set the JsonExpr->omit_quotes in the
function transformJsonFuncExpr
for case JSON_QUERY_OP and JSON_VALUE_OP.

We need to know the QUOTE behavior in the function ExecEvalJsonCoercion.
Because for ExecEvalJsonCoercion, the coercion datum source can be a
scalar string item,
scalar items means RETURNING clause is dependent on QUOTE behavior.
keep quotes, omit quotes the results are different.
consider
JSON_QUERY(jsonb'{"rec": "[1,2]"}', '$.rec' returning int4range omit quotes);
and
JSON_QUERY(jsonb'{"rec": "[1,2]"}', '$.rec' returning int4range omit quotes);

to make sure ExecEvalJsonCoercion can distinguish keep and omit quotes,
I added a bool keep_quotes to struct JsonCoercion.
(maybe there is a more simple way, so far, that's what I come up with).
the keep_quotes value will be settled in the function transformJsonFuncExpr.
After refactoring, in ExecEvalJsonCoercion, keep_quotes is true then
call JsonbToCString, else call JsonbUnquote.

example:
SELECT JSON_QUERY(jsonb'{"rec": "{1,2,3}"}', '$.rec' returning int[]
omit quotes);
without my changes, return NULL
with my changes:
 {1,2,3}

JSON_VALUE:
main changes:
--- a/src/test/regress/expected/jsonb_sqljson.out
+++ b/src/test/regress/expected/jsonb_sqljson.out
@@ -301,7 +301,11 @@ SELECT JSON_VALUE(jsonb '"2017-02-20"', '$'
RETURNING date) + 9;
 -- Test NULL checks execution in domain types
 CREATE DOMAIN sqljsonb_int_not_null AS int NOT NULL;
 SELECT JSON_VALUE(jsonb 'null', '$' RETURNING sqljsonb_int_not_null);
-ERROR:  domain sqljsonb_int_not_null does not allow null values
+ json_value
+------------
+
+(1 row)
+
I think the change is correct, given `SELECT JSON_VALUE(jsonb 'null',
'$' RETURNING int4range);` returns NULL.

I also attached a test.sql, without_patch.out (apply v33 only),
with_patch.out (my changes based on v33).
So you can see the difference after applying the patch, in case, my
wording is not clear.

---

From: jian he <[email-masked]>
Date: Wed, 3 Jan 2024 18:53:34 +0800

some more minor issues:
SELECT * FROM JSON_TABLE(jsonb '{"a":[123,2]}', '$'
    COLUMNS (item int[] PATH '$.a' error on error, foo text path '$'
error on error)) bar;
ERROR:  JSON path expression in JSON_VALUE should return singleton scalar item

the error message seems not so great, imho.
since the JSON_TABLE doc entries didn't mention that
JSON_TABLE actually transformed to json_value, json_query, json_exists.

JSON_VALUE even though cannot specify KEEP | OMIT QUOTES.
It might be a good idea to mention the default is to omit quotes  in the doc.
because JSON_TABLE actually transformed to json_value, json_query, json_exists.
JSON_TABLE can specify quotes behavior freely.

bother again, i kind of get what the function transformJsonTableChildPlan do,
but adding more comments would make it easier to understand....

(json_query)
+        This function must return a JSON string, so if the path expression
+        returns multiple SQL/JSON items, you must wrap the result using the
+        <literal>WITH WRAPPER</literal> clause. If the wrapper is
+        <literal>UNCONDITIONAL</literal>, an array wrapper will always
+        be applied, even if the returned value is already a single JSON object
+        or an array, but if it is <literal>CONDITIONAL</literal>, it
will not be
+        applied to a single array or object. <literal>UNCONDITIONAL</literal>
+        is the default.  If the result is a scalar string, by default the value
+        returned will have surrounding quotes making it a valid JSON value,
+        which can be made explicit by specifying <literal>KEEP
QUOTES</literal>.
+        Conversely, quotes can be omitted by specifying <literal>OMIT
QUOTES</literal>.
+        The returned <replaceable>data_type</replaceable> has the
same semantics
+        as for constructor functions like <function>json_objectagg</function>;
+        the default returned type is <type>jsonb</type>.

+       <para>
+        Returns the result of applying the
+        <replaceable>path_expression</replaceable> to the
+        <replaceable>context_item</replaceable> using the
+        <literal>PASSING</literal> <replaceable>value</replaceable>s. The
+        extracted value must be a single <acronym>SQL/JSON</acronym> scalar
+        item. For results that are objects or arrays, use the
+        <function>json_query</function> function instead.
+        The returned <replaceable>data_type</replaceable> has the
same semantics
+        as for constructor functions like <function>json_objectagg</function>.
+        The default returned type is <type>text</type>.
+        The <literal>ON ERROR</literal> and <literal>ON EMPTY</literal>
+        clauses have similar semantics as mentioned in the description of
+        <function>json_query</function>.
+       </para>

+        The returned <replaceable>data_type</replaceable> has the
same semantics
+        as for constructor functions like <function>json_objectagg</function>.

IMHO, the above description is not so good, since the function
json_objectagg is listed in functions-aggregate.html,
using Ctrl + F in the browser cannot find json_objectagg in functions-json.html.

for json_query, maybe we can rephrase like:
the RETURNING clause, which specifies the data type returned. It must
be a type for which there is a cast from text to that type.
By default, the <type>jsonb</type> type is returned.

json_value:
the RETURNING clause, which specifies the data type returned. It must
be a type for which there is a cast from text to that type.
By default, the <type>text</type> type is returned.

---

From: jian he <[email-masked]>
Date: Sat, 6 Jan 2024 08:44:00 +0800

some tests after applying V33 and my small changes.
setup:
create table test_scalar1(js jsonb);
insert into test_scalar1 select jsonb '{"a":"[12,13]"}' FROM
generate_series(1,1e5) g;
create table test_scalar2(js jsonb);
insert into test_scalar2 select jsonb '{"a":12}' FROM generate_series(1,1e5) g;
create table test_array1(js jsonb);
insert into test_array1 select jsonb '{"a":[1,2,3,4,5]}' FROM
generate_series(1,1e5) g;
create table test_array2(js jsonb);
insert into test_array2 select jsonb '{"a": "{1,2,3,4,5}"}' FROM
generate_series(1,1e5) g;

tests:
----------------------------------------return a scalar int4range
explain(costs off,analyze) SELECT item FROM test_scalar1,
JSON_TABLE(js, '$.a' COLUMNS (item int4range PATH '$' omit quotes))
\watch count=5
237.753 ms

explain(costs off,analyze) select json_query(js, '$.a' returning
int4range omit quotes) from test_scalar1  \watch count=5
462.379 ms

explain(costs off,analyze) select json_value(js,'$.a' returning
int4range) from test_scalar1 \watch count=5
362.148 ms

explain(costs off,analyze) select (js->>'a')::int4range from
test_scalar1 \watch count=5
301.089 ms

explain(costs off,analyze) select trim(both '"' from
jsonb_path_query_first(js,'$.a')::text)::int4range from test_scalar1
\watch count=5
643.337 ms

----------------------------return a numeric array from jsonb array.
explain(costs off,analyze) SELECT item FROM test_array1,
JSON_TABLE(js, '$.a' COLUMNS (item numeric[] PATH '$')) \watch count=5
727.807 ms

explain(costs off,analyze) SELECT json_query(js, '$.a' returning
numeric[]) from test_array1 \watch count=5
2995.909 ms

explain(costs off,analyze) SELECT
replace(replace(js->>'a','[','{'),']','}')::numeric[] from test_array1
\watch count=5
2990.114 ms

----------------------------return a numeric array from jsonb string
explain(costs off,analyze) SELECT item FROM test_array2,
JSON_TABLE(js, '$.a' COLUMNS (item numeric[] PATH '$' omit quotes))
\watch count=5
237.863 ms

explain(costs off,analyze) SELECT json_query(js,'$.a' returning
numeric[] omit quotes) from test_array2 \watch count=5
893.888 ms

explain(costs off,analyze) SELECT trim(both '"'
from(jsonb_path_query(js,'$.a')::text))::numeric[] from test_array2
\watch count=5
1329.713 ms

explain(costs off,analyze) SELECT (js->>'a')::numeric[] from
test_array2 \watch count=5
740.645 ms

explain(costs off,analyze) SELECT trim(both '"' from
(json_query(js,'$.a' returning text)))::numeric[]  from test_array2
\watch count=5
1085.230 ms
----------------------------return a scalar numeric
explain(costs off,analyze) SELECT item FROM test_scalar2,
JSON_TABLE(js, '$.a' COLUMNS (item numeric PATH '$' omit quotes)) \watch count=5
238.036 ms

explain(costs off,analyze) select json_query(js,'$.a' returning
numeric) from test_scalar2 \watch count=5
300.862 ms

explain(costs off,analyze) select json_value(js,'$.a' returning
numeric) from test_scalar2 \watch count=5
160.035 ms

explain(costs off,analyze) select
jsonb_path_query_first(js,'$.a')::numeric from test_scalar2 \watch
count=5
294.666 ms

explain(costs off,analyze) select jsonb_path_query(js,'$.a')::numeric
from test_scalar2 \watch count=5
547.130 ms

explain(costs off,analyze) select (js->>'a')::numeric from
test_scalar2 \watch count=5
243.652 ms

explain(costs off,analyze) select (js->>'a')::numeric,
(js->>'a')::numeric from test_scalar2 \watch count=5
403.183 ms

explain(costs off,analyze) select json_value(js,'$.a' returning numeric),
        json_value(js,'$.a' returning numeric) from test_scalar2 \watch count=5
246.405 ms

explain(costs off,analyze) select json_query(js,'$.a' returning numeric),
        json_query(js,'$.a' returning numeric) from test_scalar2 \watch count=5
520.754 ms

explain(costs off,analyze) SELECT item, item1 FROM test_scalar2,
JSON_TABLE(js, '$.a' COLUMNS (item numeric PATH '$' omit quotes,
                            item1 numeric PATH '$' omit quotes)) \watch count=5
242.586 ms
---------------------------------
overall, json_value is faster than json_query. but json_value can not
deal with arrays in some cases.
but as you can see, in some cases, json_value and json_query are not
as fast as our current implementation.

Here I only test simple nested levels. if you extra multiple values
from jsonb to sql type, then json_table is faster.
In almost all cases, json_table is faster.

json_table is actually called json_value_op, json_query_op under the hood.
Without json_value and json_query related code, json_table cannot be
implemented.

---

From: jian he <[email-masked]>
Date: Mon, 8 Jan 2024 08:00:00 +0800

> generate_series(1,1e5) g;
same as before, v33 plus my 4 minor changes (dot no-cfbot in previous
thread).
I realized my previous tests were wrong.
because I use build type=debug and also add a bunch of c_args.
so the following test results have no c_args, just  -Dbuildtype=release.
I actually tested several times.

----------------------------------------return a scalar int4range
explain(costs off,analyze) SELECT item FROM test_scalar1, JSON_TABLE(js,
'$.a' COLUMNS (item int4range PATH '$' omit quotes)) \watch count=5
56.487 ms

explain(costs off,analyze) select json_query(js, '$.a' returning int4range
omit quotes) from test_scalar1  \watch count=5
27.272 ms

explain(costs off,analyze) select json_value(js,'$.a' returning int4range)
from test_scalar1 \watch count=5
22.775 ms

explain(costs off,analyze) select (js->>'a')::int4range from test_scalar1
\watch count=5
17.520 ms

explain(costs off,analyze) select trim(both '"' from
jsonb_path_query_first(js,'$.a')::text)::int4range from test_scalar1 \watch
count=5
36.946 ms

----------------------------return a numeric array from jsonb array.
explain(costs off,analyze) SELECT item FROM test_array1, JSON_TABLE(js,
'$.a' COLUMNS (item numeric[] PATH '$')) \watch count=5
20.197 ms

explain(costs off,analyze) SELECT json_query(js, '$.a' returning numeric[])
from test_array1 \watch count=5
69.759 ms

62.114 ms

----------------------------return a numeric array from jsonb string
explain(costs off,analyze) SELECT item FROM test_array2,  JSON_TABLE(js,
'$.a' COLUMNS (item numeric[] PATH '$' omit quotes)) \watch count=5
18.770 ms

explain(costs off,analyze) SELECT json_query(js,'$.a' returning numeric[]
omit quotes) from test_array2 \watch count=5
46.373 ms

explain(costs off,analyze) SELECT trim(both '"'
from(jsonb_path_query(js,'$.a')::text))::numeric[] from test_array2 \watch
count=5
71.901 ms

explain(costs off,analyze) SELECT (js->>'a')::numeric[] from test_array2
\watch count=5
35.572 ms

explain(costs off,analyze) SELECT trim(both '"' from (json_query(js,'$.a'
returning text)))::numeric[]  from test_array2 \watch count=5
58.755 ms

----------------------------return a scalar numeric
explain(costs off,analyze) SELECT item FROM test_scalar2,
JSON_TABLE(js, '$.a' COLUMNS (item numeric PATH '$' omit quotes)) \watch
count=5
18.723 ms

explain(costs off,analyze) select json_query(js,'$.a' returning numeric)
from test_scalar2 \watch count=5
18.234 ms

explain(costs off,analyze) select json_value(js,'$.a' returning numeric)
from test_scalar2 \watch count=5
11.667 ms

explain(costs off,analyze) select jsonb_path_query_first(js,'$.a')::numeric
from test_scalar2 \watch count=5
17.691 ms

explain(costs off,analyze) select jsonb_path_query(js,'$.a')::numeric from
test_scalar2 \watch count=5
31.596 ms

explain(costs off,analyze) select (js->>'a')::numeric from test_scalar2
\watch count=5
13.887 ms

----------------------------return two scalar numeric
explain(costs off,analyze) select (js->>'a')::numeric, (js->>'a')::numeric
from test_scalar2 \watch count=5
22.201 ms

explain(costs off,analyze) SELECT item, item1 FROM test_scalar2,
JSON_TABLE(js, '$.a' COLUMNS (item numeric PATH '$' omit quotes,
                            item1 numeric PATH '$' omit quotes)) \watch
count=5
19.108 ms

explain(costs off,analyze) select json_value(js,'$.a' returning numeric),
        json_value(js,'$.a' returning numeric) from test_scalar2 \watch
count=5
17.915 ms

---

From: Amit Langote <[email-masked]>
Date: Tue, 16 Jan 2024 19:00:29 +0900

Hi,

Thought I'd share an update.

I've been going through Jian He's comments (thanks for the reviews!),
most of which affect the last JSON_TABLE() patch and in some cases the
query functions patch (0007).  It seems I'll need to spend a little
more time, especially on the JSON_TABLE() patch, as I'm finding things
to improve other than those mentioned in the comments.

As for the preliminary patches 0001-0006, I'm thinking that it would
be a good idea to get them out of the way sooner rather than waiting
till the main patches are in perfect shape.  I'd like to get them
committed by next week after a bit of polishing, so if anyone would
like to take a look, please let me know.  I'll post a new set
tomorrow.

0007, the query functions patch, also looks close to ready, though I
might need to change a few things in it as I work through the
JSON_TABLE() changes.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

---

From: Amit Langote <[email-masked]>
Date: Thu, 18 Jan 2024 22:12:57 +0900

Hi,

> Hi

Thanks for the reviews.

> should it be "These functions are"

Rewrote that sentence to say "introduces the following SQL/JSON functions..."

> +        an error is generated if it yields no items.

True, fixed.

> JSON_TABLE(vals.js::jsonb, '$' COLUMNS (a int PATH '$' default 1 ON

That check is to catch an ON EMPTY specified *outside* the COLUMN(...)
clause of a JSON_TABLE(...) expression.  It was added during a recent
gram.y refactoring, but maybe that wasn't a great idea.  It seems
better to disallow the ON EMPTY clause in the grammar itself.

> +   the resulting rows are joined to the <firstterm>parent</firstterm> row.

Terms "child" and "parent" are already introduced in previous
paragraphs, so no need for the <firstterm> tag.

> even though I think currently we can only do `FORMAT JSON`.

The syntax appears to allow an optional ENCODING UTF8 too, so I've
gotten rid of json_representation and literally listed out what the
syntax says.

> but these two are different things?

EMPTY and EMPTY ARRAY both spell out an array:

json_behavior_type:
...
            | EMPTY_P ARRAY { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
            /* non-standard, for Oracle compatibility only */
            | EMPTY_P   { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }

> typo, should be "formatted columns".

Oops.

> specified when WITH WRAPPER is used").

It seems to me that we should just remove the above check in
appendJsonTableColumns() and let the check(s) in parse_expr.c take
care of the various allowed/disallowed scenarios for "formatted"
columns.  Also see further below...

> so bool variable name as "left" seems not so good?

Hmm, maybe, "more" might be more appropriate given the context.

> since json_table patch is quite recursive, IMHO.

Agree that the various JsonTable parser/executor comments are lacking.
Working on adding more commentary and improving the notation -- struct
names, etc.

> transformJsonExprCommon is duplicated.

Thanks, I've adopted some of the ideas in your patch.

On Mon, Dec 25, 2023 at 2:03 PM jian he <[email-masked]> wrote:
> +/*
> + * JsonTableFetchRow
> + * Prepare the next "current" tuple for upcoming GetValue calls.
> + * Returns FALSE if the row-filter expression returned no more rows.
> + */
> +static bool
> +JsonTableFetchRow(TableFuncScanState *state)
> +{
> + JsonTableExecContext *cxt =
> + GetJsonTableExecContext(state, "JsonTableFetchRow");
> +
> + if (cxt->empty)
> + return false;
> +
> + return JsonTableScanNextRow(cxt->root);
> +}
>
> The declaration of struct JsonbTableRoutine, SetRowFilter field is
> null. So I am confused by the above comment.

Yeah, it might be a leftover from copy-pasting the XML code.  Reworded
the comment to not mention SetRowFilter.

> also seems the  `if (cxt->empty)` part never called.

I don't understand why the context struct has that empty flag too, it
might be a leftover field.  Removed.

> +static inline JsonTableExecContext *
> +GetJsonTableExecContext(TableFuncScanState *state, const char *fname)
> +{
> + JsonTableExecContext *result;
> +
> + if (!IsA(state, TableFuncScanState))
> + elog(ERROR, "%s called with invalid TableFuncScanState", fname);
> + result = (JsonTableExecContext *) state->opaque;
> + if (result->magic != JSON_TABLE_EXEC_CONTEXT_MAGIC)
> + elog(ERROR, "%s called with invalid TableFuncScanState", fname);
> +
> + return result;
> +}
> I think Assert(IsA(state, TableFuncScanState)) would be better.

Hmm, better to leave this as-is to be consistent with what the XML
code is doing.  Though I also wonder why it's not an Assert in the
first place.

> +/*
> + * JsonTablePlanType -
> + * flags for JSON_TABLE plan node types representation
> + */
> +typedef enum JsonTablePlanType
> +{
> + JSTP_DEFAULT,
> + JSTP_SIMPLE,
> + JSTP_JOINED,
> +} JsonTablePlanType;
> it would be better to add some comments on it. thanks.
>
> JsonTablePlanNextRow is quite recursive! Adding more explanation would
> be helpful, thanks.

Will do.

> +/* Recursively reset scan and its child nodes */
> +static void
> +JsonTableRescanRecursive(JsonTablePlanState * state)
> +{
> + if (state->type == JSON_TABLE_JOIN_STATE)
> + {
> + JsonTableJoinState *join = (JsonTableJoinState *) state;
> +
> + JsonTableRescanRecursive(join->left);
> + JsonTableRescanRecursive(join->right);
> + join->advanceRight = false;
> + }
> + else
> + {
> + JsonTableScanState *scan = (JsonTableScanState *) state;
> +
> + Assert(state->type == JSON_TABLE_SCAN_STATE);
> + JsonTableRescan(scan);
> + if (scan->plan.nested)
> + JsonTableRescanRecursive(scan->plan.nested);
> + }
> +}
>
> From the coverage report, I noticed the first IF branch in
> JsonTableRescanRecursive never called.

Will look into this.

> + foreach(col, columns)
> + {
> + JsonTableColumn *rawc = castNode(JsonTableColumn, lfirst(col));
> + Oid typid;
> + int32 typmod;
> + Node   *colexpr;
> +
> + if (rawc->name)
> + {
> + /* make sure column names are unique */
> + ListCell   *colname;
> +
> + foreach(colname, tf->colnames)
> + if (!strcmp((const char *) colname, rawc->name))
> + ereport(ERROR,
> + errmsg("column name \"%s\" is not unique",
> + rawc->name),
> this `/* make sure column names are unique */` logic part already
> validated in isJsonTablePathNameDuplicate, so we don't need it?
> actually isJsonTablePathNameDuplicate validates both column name and pathname.

I think you are right.  All columns/path names are de-duplicated much
earlier at the beginning of transformJsonTable(), so there's no need
for the above check.

That said, I don't know why column and path names share the namespace
or whether that has any semantic issues.  Maybe there aren't, but will
think some more on that.

> select jt.* from jsonb_table_test jtt,
> json_table (jtt.js,'strict $[*]' as p
> columns (n for ordinality,
> nested path 'strict $.b[*]' as pb columns ( c int path '$' ),
> nested path 'strict $.b[*]' as pb columns ( s int path '$' ))
> ) jt;
>
> ERROR:  duplicate JSON_TABLE column name: pb
> HINT:  JSON_TABLE column names must be distinct from one another.
> the error is not very accurate, since pb is a pathname?

I think this can be improved by passing the information whether it's a
column or path name to the deduplication code.  I've reworked that
code to get more useful error info.

> JSON_TABLE actually transformed to json_value, json_query, json_exists.

Hmm, yes, the context whether the JSON_VALUE() is user-specified or
internally generated is not readily available where the error is
reported.

I'm inlinced to document this aspect of JSON_TABLE(), instead of
complicating the executor interfaces in order to make the error
message better.

> JSON_TABLE can specify quotes behavior freely.

Done.

> By default, the <type>text</type> type is returned.

Fixed the description of returned type for both json_query() and
json_value().  For the latter, the cast to the returned type must
exist from each possible JSON scalar type viz. text, boolean, numeric,
and various datetime types.

> overall, json_table behavior is more consistent with json_query and json_value.

Thanks for the patches.  I've taken the tests, some of your suggested
code changes, and made some changes of my own.  Some of the new tests
give a different error message than what your patch had but I think
what I have is fine.

> I also add asserts to it.

I realized that we don't really need the via_io and via_populate
flags.  You can see in the latest patch that the decision of whether
to call json_populate_type() or the RETURNING type's input function is
now deferred to run-time or ExecEvalJsonCoercion().  The new comment
should also make it clear why one or the other is used for a given
source datum passed to ExecEvalJsonCoercion().

> -ERROR:  domain sqljsonb_int_not_null does not allow null values

To address these points:

* I've taken your idea to make omit/keep_quotes available to
ExecEvalJsonCoercion().

* I've also taken your suggestion to fix parse_jsontable.c such that
WRAPPER/QUOTES combinations specified with JSON_TABLE() columns work
without many arbitrary-looking restrictions.

Please take a look at the attached latest patch and let me know if
anything looks amiss.

> jsonb_path_query_first(js,'$.a')::text)::int4range from test_scalar1

I think that may be explained by the fact that JsonPathQuery() has
this step, which JsonPathValue() does not:

    if (singleton)
        return JsonbPGetDatum(JsonbValueToJsonb(singleton));

I can see JsonbValueToJsonb() in perf profile when running the
benchmark you shared.  I don't know if there's any way to make that
better.

> as fast as our current implementation

Yeah, there *is* some expected overhead to using the new functions;
ExecEvalJsonExprPath() appears in the top 5 frames of perf profile,
for example.  The times I see are similar to yours and I don't find
the difference to be very drastic.

postgres=# \o /dev/null
postgres=# explain(costs off,analyze) select (js->>'a') from
test_scalar1 \watch count=3
Time: 21.581 ms
Time: 18.838 ms
Time: 21.589 ms

postgres=# explain(costs off,analyze) select json_query(js,'$.a') from
test_scalar1 \watch count=3
Time: 38.562 ms
Time: 34.251 ms
Time: 32.681 ms

postgres=# explain(costs off,analyze) select json_value(js,'$.a') from
test_scalar1 \watch count=3
Time: 28.595 ms
Time: 23.947 ms
Time: 25.334 ms

postgres=# explain(costs off,analyze) select item from test_scalar1,
json_table(js, '$.a' columns (item int4range path '$')); \watch
count=3
Time: 52.739 ms
Time: 53.996 ms
Time: 50.774 ms

Attached v34 of all of the patches.  0008 may be considered to be WIP
given the points I mentioned above -- need to add a bit more
commentary about JSON_TABLE plan implementation and other
miscellaneous fixes.

As said in my previous email, I'd like to commit 0001-0007 next week.

---

From: Amit Langote <[email-masked]>
Date: Thu, 18 Jan 2024 22:35:41 +0900

> commentary about JSON_TABLE plan implementation and other

Oops, I had forgotten to update the ECPG test's expected output in
0008.  Fixed in the attached.