Thank you for review.
Attached 45th version of the patches. "SQL/JSON functions" patch corresponds to
v52 patch set posted in the separate thread.
On 27.12.2020 01:16, Zhihong Yu wrote:
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.
Fixed.
For transformJsonTableColumn:
+ jfexpr->op =
+ 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 ?
Added mention of EXISTS PATH column to the comment.
For JsonTableDestroyOpaque():
+ state->opaque = NULL;
Should the memory pointed to by opaque be freed ?
I think it's not necessary, because FunctionScan, caller of
JsonTableDestroyOpaque(), will free it and also all opaque's fields using
MemoryContextReset().
+ l2 = list_head(tf->coltypes);
+ 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.
Variable were renamed, also foreach() loop was refactored using forfour() macro.
+typedef enum JsonTablePlanJoinType
+{
+ 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)
since different fields are checked in the two if statements but the prefixes don't convey that.
Enumeration members were renames using prefix JSTPJ_.
+ Even though the path names are not incuded into the <literal>PLAN DEFAULT</literal>
Typo: incuded -> included
Fixed.
+ int nchilds = 0;
nchilds -> nchildren
Fixed.
+#if 0 /* XXX it' unclear from the standard whether root path name is mandatory or not */
+ if (plan && plan->plan_type != JSTP_DEFAULT && !rootPathName)
Do you plan to drop the if block ?
If it becomes clear, I will drop it.
