Re: SQL/JSON: JSON_TABLE

> On 22 Mar 2022, at 16:31, Andrew Dunstan <andrew@dunslane.net> wrote:

> I'm planning on pushing the functions patch set this week and json-table
> next week.

My comments from 30827B3C-EDF6-4D41-BBF1-2981818744A8@yesql.se are yet to be
addressed (or at all responded to) in this patchset.  I'll paste the ones which
still apply to make it easier:

+ into both child and parrent columns for all missing values.
s/parrent/parent/

-           objectname = "xmltable";
+           objectname = rte->tablefunc ?
+               rte->tablefunc->functype == TFT_XMLTABLE ?
+               "xmltable" : "json_table" : NULL;
In which case can rte->tablefunc be NULL for a T_TableFuncScan? Also, nested
ternary operators are confusing at best, I think this should be rewritten as
plain if statements.

In general when inspecting functype I think it's better to spell it out with if
statements rather than ternary since it allows for grepping the code easier.
Having to grep for TFT_XMLTABLE to find where TFT_JSON_TABLE could be used
isn't all that convenient.

+ errmsg("JSON_TABLE() is not yet implemented for json type"),
I can see this being potentially confusing to many, en errhint with a reference
to jsonb seems like a good idea.

+/* Recursively register column name in the path name list. */
+static void
+registerJsonTableColumn(JsonTableContext *cxt, char *colname)
This comment is IMO misleading since the function isn't actually recursive, but a
helper function for a recursive function.

+   switch (get_typtype(typid))
+   {
+       case TYPTYPE_COMPOSITE:
+           return true;
+
+       case TYPTYPE_DOMAIN:
+           return typeIsComposite(getBaseType(typid));
+   }
switch statements without a default runs the risk of attracting unwanted
compiler warning attention, or make static analyzers angry. This one can
easily be rewritten with two if-statements on a cached get_typtype()
returnvalue.

+ * Returned false at the end of a scan, true otherwise.
s/Returned/Returns/ (applies at two places)

+ /* state->ordinal--; */ /* skip current outer row, reset counter */
Is this dead code to be removed, or left in there as a reminder to fix
something?

--
Daniel Gustafsson        https://vmware.com/