Re: remaining sql/json patches

On Wed, Apr 3, 2024 at 4:16 PM jian he <jian.universality@gmail.com> wrote:
> On Wed, Apr 3, 2024 at 11:30 AM jian he <jian.universality@gmail.com> wrote:
> >
> > On Tue, Apr 2, 2024 at 9:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > >
> > > Please let me know if you have further comments on 0001.  I'd like to
> > > get that in before spending more energy on 0002.
> > >
>
> -- a/src/backend/parser/parse_target.c
> +++ b/src/backend/parser/parse_target.c
> @@ -2019,6 +2019,9 @@ FigureColnameInternal(Node *node, char **name)
>   case JSON_VALUE_OP:
>   *name = "json_value";
>   return 2;
> + case JSON_TABLE_OP:
> + *name = "json_table";
> + return 2;
>   default:
>   elog(ERROR, "unrecognized JsonExpr op: %d",
>   (int) ((JsonFuncExpr *) node)->op);
>
> "case JSON_TABLE_OP part", no need?
> json_table output must provide column name and type?

That seems to be the case, so removed.

> I did some minor refactor transformJsonTableColumns, make the comments
> align with the function intention.

Thanks, but that seems a bit verbose.  I've reduced it down to what
gives enough information.

> in v48-0001, in transformJsonTableColumns we can `Assert(rawc->name);`.
> since non-nested JsonTableColumn must specify column name.
> in v48-0002, we can change to `if (rawc->coltype != JTC_NESTED)
> Assert(rawc->name);`

Ok, done.

> SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int PATH '$.a' )
> ERROR ON ERROR) jt;
> ERROR:  no SQL/JSON item
>
> I thought it should just return NULL.
> In this case, I thought that
> (not column-level) ERROR ON ERROR should not interfere with "COLUMNS
> (a int PATH '$.a' )".

I think it does in another database's implementation, which must be
why the original authors decided that the table-level ERROR should
also be used for columns unless overridden.  But I agree that keeping
the two separate is better, so changed that way.

Attached updated patches.  I have addressed your doc comments on 0001,
but not 0002 yet.


>
> +-- Other miscellanous checks
> "miscellanous" should be "miscellaneous".
>
>
> overall the coverage is pretty high.
> the current test output looks fine.



--
Thanks, Amit Langote