Thread: Re: SQL/JSON json_table plan clause

Hi Nikita,

I was looking at the patch you posted on Feb 3. Two high-level comments:

* The documentation additions are missing.

* It looks like the patch mixes refactoring changes with new
functionality, which makes it harder to follow what's existing vs.
new. Could you separate these changes so the new additions are easier
to review?

More specifically on the 2nd point:

-/*
- * Check if the type is "composite" for the purpose of checking whether to use
- * JSON_VALUE() or JSON_QUERY() for a given JsonTableColumn.
- */
-static bool
-isCompositeType(Oid typid)
-{
-    char        typtype = get_typtype(typid);
-
-    return typid == JSONOID ||
-        typid == JSONBOID ||
-        typid == RECORDOID ||
-        type_is_array(typid) ||
-        typtype == TYPTYPE_COMPOSITE ||
-    /* domain over one of the above? */
-        (typtype == TYPTYPE_DOMAIN &&
-         isCompositeType(getBaseType(typid)));
 }
...

Diffs like this one make me wonder whether this code actually needs to
change for PLAN clause support -- but I suspect it does not.

+
+    /**/
+    bool        cross;
+    bool        outerJoin;
+    bool        advanceNested;
+    bool        advanceRight;
+    bool        reset;

Incomplete comment.

@@ -4124,6 +4130,7 @@ JsonTableInitOpaque(TableFuncScanState *state, int natts)
      * Evaluate JSON_TABLE() PASSING arguments to be passed to the jsonpath
      * executor via JsonPathVariables.
      */
+
     if (state->passingvalexprs)
@@ -4155,15 +4162,13 @@ JsonTableInitOpaque(TableFuncScanState *state,
int natts)
     }

     cxt->colplanstates = palloc(sizeof(JsonTablePlanState *) *
-                                list_length(tf->colvalexprs));
-
+                               list_length(tf->colvalexprs));
     /*
      * Initialize plan for the root path and, recursively, also any child
      * plans that compute the NESTED paths.
      */
     cxt->rootplanstate = JsonTableInitPlan(cxt, rootplan, NULL, args,
-                                           CurrentMemoryContext);
-
+                                       CurrentMemoryContext);
     state->opaque = cxt;
 }

@@ -4201,8 +4206,9 @@ JsonTableInitPlan(JsonTableExecContext *cxt,
JsonTablePlan *plan,
     if (IsA(plan, JsonTablePathScan))
     {
         JsonTablePathScan *scan = (JsonTablePathScan *) plan;
-        int         i;
+        int     i;

Unnecessary whitespace addition/removal.

 /*
- * Transform JSON_TABLE column definition into a JsonFuncExpr
- * This turns:
+ * Transform JSON_TABLE column

Not sure why the comment was rewritten.

-JsonTableResetRowPattern(JsonTablePlanState *planstate, Datum item)
+JsonTableResetContextItem(JsonTablePlanState *planstate, Datum item)

I remember renaming ContextItem with RowPattern, but it seems this is
switching it back.

It makes it look like you're reverting to code from the old patch that
implemented the PLAN clause together with JSON_TABLE(), instead of
building on the committed code and adding just what's needed to
support PLAN clause in JsonTablePlan.  Especially, the changes to
parse_jsontable.c and jsonpath_exec.c,