Re: remaining sql/json patches

On Wed, Apr 3, 2024 at 11:48 PM jian he <jian.universality@gmail.com> wrote:
> hi.
> +  <para>
> +   <function>json_table</function> is an SQL/JSON function which
> +   queries <acronym>JSON</acronym> data
> +   and presents the results as a relational view, which can be accessed as a
> +   regular SQL table. You can only use
> <function>json_table</function> inside the
> +   <literal>FROM</literal> clause of a <literal>SELECT</literal>,
> +   <literal>UPDATE</literal>, <literal>DELETE</literal>, or
> <literal>MERGE</literal>
> +   statement.
> +  </para>
>
> the only issue is that <literal>MERGE</literal> Synopsis don't have
> <literal>FROM</literal> clause.
> other than that, it's quite correct.
> see following tests demo:
>
> drop table ss;
> create table ss(a int);
> insert into ss select 1;
> delete from ss using JSON_TABLE(jsonb '1', '$' COLUMNS (a int PATH '$'
> ) ERROR ON ERROR) jt where jt.a = 1;
> insert into ss select 2;
> update ss set a = 1 from JSON_TABLE(jsonb '2', '$' COLUMNS (a int PATH
> '$')) jt where jt.a = 2;
> DROP TABLE IF EXISTS target;
> CREATE TABLE target (tid integer, balance integer) WITH
> (autovacuum_enabled=off);
> INSERT INTO target VALUES (1, 10),(2, 20),(3, 30);
> MERGE INTO target USING JSON_TABLE(jsonb '2', '$' COLUMNS (a int PATH
> '$' ) ERROR ON ERROR) source(sid)
> ON target.tid = source.sid
> WHEN MATCHED THEN UPDATE SET balance = 0
> returning *;
> --------------------------------------------------------------------------------------------------
>
> +  <para>
> +   To split the row pattern into columns, <function>json_table</function>
> +   provides the <literal>COLUMNS</literal> clause that defines the
> +   schema of the created view. For each column, a separate path expression
> +   can be specified to be evaluated against the row pattern to get a
> +   SQL/JSON value that will become the value for the specified column in
> +   a given output row.
> +  </para>
> should be "an SQL/JSON".
>
> +    <para>
> +     Inserts a SQL/JSON value obtained by applying
> +     <replaceable>path_expression</replaceable> against the row pattern into
> +     the view's output row after coercing it to specified
> +     <replaceable>type</replaceable>.
> +    </para>
> should be "an SQL/JSON".
>
> "coercing it to specified <replaceable>type</replaceable>"
> should be
> "coercing it to the specified <replaceable>type</replaceable>"?
> ---------------------------------------------------------------------------------------------------------------
> +    <para>
> +     The value corresponds to whether evaluating the <literal>PATH</literal>
> +     expression yields any SQL/JSON values.
> +    </para>
> maybe we can change to
> +    <para>
> +     The value corresponds to whether applying the
> <replaceable>path_expression</replaceable>
> +     expression yields any SQL/JSON values.
> +    </para>
> so it looks more consistent with the preceding paragraph.
>
> +    <para>
> +     Optionally, <literal>ON ERROR</literal> can be used to specify whether
> +     to throw an error or return the specified value to handle structural
> +     errors, respectively.  The default is to return a boolean value
> +     <literal>FALSE</literal>.
> +    </para>
> we don't need "respectively" here?
>
> + if (jt->on_error &&
> + jt->on_error->btype != JSON_BEHAVIOR_ERROR &&
> + jt->on_error->btype != JSON_BEHAVIOR_EMPTY &&
> + jt->on_error->btype != JSON_BEHAVIOR_EMPTY_ARRAY)
> + ereport(ERROR,
> + errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("invalid ON ERROR behavior"),
> + errdetail("Only EMPTY or ERROR is allowed for ON ERROR in JSON_TABLE()."),
> + parser_errposition(pstate, jt->on_error->location));
>
> errdetail("Only EMPTY or ERROR is allowed for ON ERROR in JSON_TABLE()."),
> maybe change to something like:
> `
> errdetail("Only EMPTY or ERROR is allowed for ON ERROR in the
> top-level JSON_TABLE() ").
> `
> i guess mentioning "top-level" is fine.
> since "top-level", we have 19  appearances in functions-json.html.

Thanks for checking.

Pushed after fixing these and a few other issues.  I didn't include
the testing function you proposed in your other email.  It sounds
useful for testing locally but will need some work before we can
include it in the tree.

I'll post the rebased 0002 tomorrow after addressing your comments.


--
Thanks, Amit Langote