Below are a few small comments from a casual read-through. I noticed that
there was a new version posted after I had finished perusing, but it seems to
address other aspects.
+ Gerenates a column and inserts a composite SQL/JSON
s/Gerenates/Generates/
+ 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 json_table isn't all that convenient.
That also removes the need for comments stating why a ternary operator is Ok in
the first place.
+ 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 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/
