Re: SQL/JSON: JSON_TABLE
| От | Zhihong Yu |
|---|---|
| Тема | Re: SQL/JSON: JSON_TABLE |
| Дата | |
| Msg-id | CALNJ-vTXxYOy9baMhFRQH9S5KUkuy8UfO4EpjQv77H7ymcN6nA@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: SQL/JSON: JSON_TABLE (Nikita Glukhov <n.gluhov@postgrespro.ru>) |
| Ответы |
Re: SQL/JSON: JSON_TABLE
|
| Список | pgsql-hackers |
For new files introduced in the patches:
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
2021 is a few days ahead. It would be good to update the year range.
For transformJsonTableColumn:
+ jfexpr->op =
+ jtc->coltype == JTC_REGULAR ? IS_JSON_VALUE :
+ jtc->coltype == JTC_EXISTS ? IS_JSON_EXISTS : IS_JSON_QUERY;
+ jtc->coltype == JTC_REGULAR ? IS_JSON_VALUE :
+ jtc->coltype == JTC_EXISTS ? IS_JSON_EXISTS : IS_JSON_QUERY;
Should IS_JSON_EXISTS be mentioned in the comment preceding the method ?
For JsonTableDestroyOpaque():
+ state->opaque = NULL;
Should the memory pointed to by opaque be freed ?
+ l2 = list_head(tf->coltypes);
+ l3 = list_head(tf->coltypmods);
+ l4 = list_head(tf->colvalexprs);
+ l3 = list_head(tf->coltypmods);
+ l4 = list_head(tf->colvalexprs);
Maybe the ListCell pointer variables can be named corresponding to the lists they iterate so that the code is easier to understand.
+typedef enum JsonTablePlanJoinType
+{
+ JSTP_INNER = 0x01,
+ JSTP_OUTER = 0x02,
+ JSTP_CROSS = 0x04,
+{
+ JSTP_INNER = 0x01,
+ JSTP_OUTER = 0x02,
+ JSTP_CROSS = 0x04,
Since plan type enum starts with JSTP_, I think the plan join type should start with JSTPJ_ or JSTJ_. Otherwise the following would be a bit hard to read:
+ else if (plan->plan_type == JSTP_JOINED)
+ {
+ if (plan->join_type == JSTP_INNER ||
+ plan->join_type == JSTP_OUTER)
+ {
+ if (plan->join_type == JSTP_INNER ||
+ plan->join_type == JSTP_OUTER)
since different fields are checked in the two if statements but the prefixes don't convey that.
+ Even though the path names are not incuded into the <literal>PLAN DEFAULT</literal>
Typo: incuded -> included
+ int nchilds = 0;
nchilds -> nchildren
+#if 0 /* XXX it' unclear from the standard whether root path name is mandatory or not */
+ if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)
+ if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)
Do you plan to drop the if block ?
Cheers
On Fri, Dec 25, 2020 at 12:32 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
On 03.08.2020 10:55, Michael Paquier wrote:On Sun, Jul 05, 2020 at 12:15:58PM -0500, Justin Pryzby wrote:It looks like this needs to be additionally rebased - I will set cfbot to "Waiting".... Something that has not happened in four weeks, so this is marked as returned with feedback. Please feel free to resubmit once a rebase is done. -- MichaelAtatched 44th version of the pacthes rebased onto current master (#0001 corresponds to v51 of SQL/JSON patches).
--
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
В списке pgsql-hackers по дате отправления: