Re: SQL/JSON: JSON_TABLE

On 2/9/22 08:22, Himanshu Upadhyaya wrote:
> On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>>
>> rebased with some review comments attended to.
> I am in process of reviewing these patches, initially, have started
> with 0002-JSON_TABLE-v55.patch.
> Tested many different scenarios with various JSON messages and these
> all are working as expected. Just one question on the below output.
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON EMPTY)) jt;
> a
> ---
>
> (1 row)
>
> ‘postgres[1406146]=#’SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS
> (a int PATH '$.a' ERROR ON ERROR)) jt;
> a
> ---
>
> (1 row)
>
> is not "ERROR ON ERROR" is expected to give error?
>
> There are a few minor comments on the patch:
> 1)
> Few Typo
> + Sibling columns are joined using
> + <literal>FULL OUTER JOIN ON FALSE</literal>, so that both
> parent and child
> + rows are included into the output, with NULL values inserted
> + into both child and parrent columns for all missing values.
>
> Parrent should be parent.
>
> + <para>
> + Gerenates a column and inserts a boolean item into each row of
> this column.
> + </para>
> Gerenates should be Generates.
>
> + <para>
> + Extracts SQL/JSON items from nested levels of the row pattern,
> + gerenates one or more columns as defined by the <literal>COLUMNS</literal>
> + subclause, and inserts the extracted SQL/JSON items into each
> row of these columns.
> + The <replaceable>json_table_column</replaceable> expression in the
> + <literal>COLUMNS</literal> subclause uses the same syntax as in the
> + parent <literal>COLUMNS</literal> clause.
> + </para>
>
> Gerenates should be Generates.
>
> +/*-------------------------------------------------------------------------
> + *
> + * parse_jsontable.c
> + * pasring of JSON_TABLE
>
> pasring should be parsing.
>
> 2) Albhabatic include.
> +
> +#include "postgres.h"
> +
> +#include "miscadmin.h"
> +
> include files are not in alphabetic order.
>
> 3)
> +-- JSON_TABLE: nested paths and plans
> +-- Should fail (column names anf path names shall be distinct)
> +SELECT * FROM JSON_TABLE(
> + jsonb '[]', '$'
> + COLUMNS (
> + a int,
> + b text,
> + a jsonb
> + )
> +) jt;
> +ERROR: duplicate JSON_TABLE column name: a
> +HINT: JSON_TABLE path names and column names shall be distinct from
> one another
>
> HINT is not much relevant, can't we simply say "JSON_TABLE column
> names should be distinct from one another"?
>
> 4)
> @@ -4837,6 +4844,7 @@ ExecEvalJsonExprSubtrans(JsonFunc func, ExprEvalStep *op,
> /* Want to execute expressions inside function's memory context */
> MemoryContextSwitchTo(oldcontext);
>
> +
>
> we can remove this empty line.
>
> 5)
> /*
> * The production for a qualified relation name has to exactly match the
> * production for a qualified func_name, because in a FROM clause we cannot
> * tell which we are parsing until we see what comes after it ('(' for a
> * func_name, something else for a relation). Therefore we allow 'indirection'
> * which may contain subscripts, and reject that case in the C code.
> */
>
> I think the sentence "because in a FROM clause we cannot
> * tell which we are parsing..." should be changed to "because in a
> FROM clause we cannot
> * tell what we are parsing "
>
> 6)
> @@ -696,7 +696,7 @@ transformRangeTableFunc(ParseState *pstate,
> RangeTableFunc *rtf)
> char **names;
> int colno;
>
> - /* Currently only XMLTABLE is supported */
>
> can we change(and not remove) the comment to "/* Currently only
> XMLTABLE and JSON_TABLE is supported */"
>
> 7)
> /*
> * TableFunc - node for a table function, such as XMLTABLE.
> *
> * Entries in the ns_names list are either String nodes containing
> * literal namespace names, or NULL pointers to represent DEFAULT.
> */
> typedef struct TableFunc
>
> can we change the comment to "...such as XMLTABLE or JSON_TABLE."?
>

This set of patches deals with items 1..7 above, but not yet the ERROR
ON ERROR issue. It also makes some message cleanups, but there is more
to come in that area.

It is based on the latest SQL/JSON Functions patch set, which does not
include the sql_json GUC patch.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com