Re: SQL/JSON: JSON_TABLE - Mailing list pgsql-hackers

On Wed, Feb 2, 2022 at 12:44 AM Andrew Dunstan <andrew@dunslane.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."?

--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com