From: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
---|---|
To: | Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> |
Cc: | Andrew Dunstan <andrew(at)dunslane(dot)net>, Erik Rijkers <er(at)xs4all(dot)nl>, Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru>, David Steele <david(at)pgmasters(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Michael Paquier <michael(at)paquier(dot)xyz>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Oleg Bartunov <obartunov(at)gmail(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com> |
Subject: | Re: SQL/JSON: JSON_TABLE |
Date: | 2021-07-26 14:12:41 |
Message-ID: | 30827B3C-EDF6-4D41-BBF1-2981818744A8@yesql.se |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2021-07-26 14:13:09 | Re: when the startup process doesn't (logging startup delays) |
Previous Message | Masahiko Sawada | 2021-07-26 14:01:46 | Re: [PoC] Improve dead tuple storage for lazy vacuum |