Re: SQL/JSON: JSON_TABLE

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."?

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