> On 22 Mar 2022, at 16:31, Andrew Dunstan <and...@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-298181874...@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/