Re: remaining sql/json patches

On Fri, Mar 22, 2024 at 12:08 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Wed, Mar 20, 2024 at 9:53 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > I'll push 0001 tomorrow.
>
> Pushed that one.  Here's the remaining JSON_TABLE() patch.
>

hi. minor issues i found json_table patch.

+ if (!IsA($5, A_Const) ||
+ castNode(A_Const, $5)->val.node.type != T_String)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("only string constants are supported in JSON_TABLE"
+   " path specification"),
+ parser_errposition(@5));
as mentioned in upthread, this error message should be one line.


+const TableFuncRoutine JsonbTableRoutine =
+{
+ JsonTableInitOpaque,
+ JsonTableSetDocument,
+ NULL,
+ NULL,
+ NULL,
+ JsonTableFetchRow,
+ JsonTableGetValue,
+ JsonTableDestroyOpaque
+};
should be:

const TableFuncRoutine JsonbTableRoutine =
{
.InitOpaque = JsonTableInitOpaque,
.SetDocument = JsonTableSetDocument,
.SetNamespace = NULL,
.SetRowFilter = NULL,
.SetColumnFilter = NULL,
.FetchRow = JsonTableFetchRow,
.GetValue = JsonTableGetValue,
.DestroyOpaque = JsonTableDestroyOpaque
};

+/*
+ * JsonTablePathSpec
+ * untransformed specification of JSON path expression with an optional
+ * name
+ */
+typedef struct JsonTablePathSpec
+{
+ NodeTag type;
+
+ Node   *string;
+ char   *name;
+ int name_location;
+ int location; /* location of 'string' */
+} JsonTablePathSpec;
the comment still does not explain the distinction between "location"
and "name_location"?


JsonTablePathSpec needs to be added to typedefs.list.
JsonPathSpec should be removed from typedefs.list.


+/*
+ * JsonTablePlanType -
+ * flags for JSON_TABLE plan node types representation
+ */
+typedef enum JsonTablePlanType
+{
+ JSTP_DEFAULT,
+ JSTP_SIMPLE,
+ JSTP_JOINED,
+} JsonTablePlanType;
+
+/*
+ * JsonTablePlanJoinType -
+ * JSON_TABLE join types for JSTP_JOINED plans
+ */
+typedef enum JsonTablePlanJoinType
+{
+ JSTP_JOIN_INNER,
+ JSTP_JOIN_OUTER,
+ JSTP_JOIN_CROSS,
+ JSTP_JOIN_UNION,
+} JsonTablePlanJoinType;
I can guess the enum value meaning of JsonTablePlanJoinType,
but I can't guess the meaning of "JSTP_SIMPLE" or "JSTP_JOINED".
adding some comments in JsonTablePlanType would make it more clear.

I think I can understand JsonTableScanNextRow.
but i don't understand JsonTablePlanNextRow.
maybe we can add some comments on JsonTableJoinState.


+-- unspecified plan (outer, union)
+select
+ jt.*
+from
+ jsonb_table_test jtt,
+ json_table (
+ jtt.js,'strict $[*]' as p
+ columns (
+ n for ordinality,
+ a int path 'lax $.a' default -1 on empty,
+ nested path 'strict $.b[*]' as pb columns ( b int path '$' ),
+ nested path 'strict $.c[*]' as pc columns ( c int path '$' )
+ )
+ ) jt;
+ n | a  | b | c
+---+----+---+----
+ 1 |  1 |   |
+ 2 |  2 | 1 |
+ 2 |  2 | 2 |
+ 2 |  2 | 3 |
+ 2 |  2 |   | 10
+ 2 |  2 |   |
+ 2 |  2 |   | 20
+ 3 |  3 | 1 |
+ 3 |  3 | 2 |
+ 4 | -1 | 1 |
+ 4 | -1 | 2 |
+(11 rows)
+
+-- default plan (outer, union)
+select
+ jt.*
+from
+ jsonb_table_test jtt,
+ json_table (
+ jtt.js,'strict $[*]' as p
+ columns (
+ n for ordinality,
+ a int path 'lax $.a' default -1 on empty,
+ nested path 'strict $.b[*]' as pb columns ( b int path '$' ),
+ nested path 'strict $.c[*]' as pc columns ( c int path '$' )
+ )
+ plan default (outer, union)
+ ) jt;
+ n | a  | b | c
+---+----+---+----
+ 1 |  1 |   |
+ 2 |  2 | 1 | 10
+ 2 |  2 | 1 |
+ 2 |  2 | 1 | 20
+ 2 |  2 | 2 | 10
+ 2 |  2 | 2 |
+ 2 |  2 | 2 | 20
+ 2 |  2 | 3 | 10
+ 2 |  2 | 3 |
+ 2 |  2 | 3 | 20
+ 3 |  3 |   |
+ 4 | -1 |   |
+(12 rows)
these two query results should be the same, if i understand it correctly.