Re: remaining sql/json patches

Thanks Alvaro.

On Wed, Dec 6, 2023 at 7:43 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2023-Dec-05, Amit Langote wrote:
>
> > I've attempted to trim down the JSON_TABLE grammar (0004), but this is
> > all I've managed so far.  Among other things, I couldn't refactor the
> > grammar to do away with the following:
> >
> > +%nonassoc  NESTED
> > +%left      PATH
>
> To recap, the reason we're arguing about this is that this creates two
> new precedence classes, which are higher than everything else.  Judging
> by the discussios in thread [1], this is not acceptable.  Without either
> those new classes or the two hacks I describe below, the grammar has the
> following shift/reduce conflict:
>
> State 6220
>
>   2331 json_table_column_definition: NESTED . path_opt Sconst COLUMNS '(' json_table_column_definition_list ')'
>   2332                             | NESTED . path_opt Sconst AS name COLUMNS '(' json_table_column_definition_list ')'
>   2636 unreserved_keyword: NESTED .
>
>     PATH  shift, and go to state 6286
>
>     SCONST    reduce using rule 2336 (path_opt)
>     PATH      [reduce using rule 2636 (unreserved_keyword)]
>     $default  reduce using rule 2636 (unreserved_keyword)
>
>     path_opt  go to state 6287
>
>
>
> First, while the grammar uses "NESTED path_opt" in the relevant productions, I
> noticed that there's no test that uses NESTED without PATH, so if we break that
> case, we won't notice.  I propose we remove the PATH keyword from one of
> the tests in jsonb_sqljson.sql in order to make sure the grammar
> continues to work after whatever hacking we do:
>
> diff --git a/src/test/regress/expected/jsonb_sqljson.out b/src/test/regress/expected/jsonb_sqljson.out
> index 7e8ae6a696..8fd2385cdc 100644
> --- a/src/test/regress/expected/jsonb_sqljson.out
> +++ b/src/test/regress/expected/jsonb_sqljson.out
> @@ -1548,7 +1548,7 @@ HINT:  JSON_TABLE column names must be distinct from one another.
>  SELECT * FROM JSON_TABLE(
>         jsonb 'null', '$[*]' AS p0
>         COLUMNS (
> -               NESTED PATH '$' AS p1 COLUMNS (
> +               NESTED '$' AS p1 COLUMNS (
>                         NESTED PATH '$' AS p11 COLUMNS ( foo int ),
>                         NESTED PATH '$' AS p12 COLUMNS ( bar int )
>                 ),
> diff --git a/src/test/regress/sql/jsonb_sqljson.sql b/src/test/regress/sql/jsonb_sqljson.sql
> index ea5db88b40..ea9b4ff8b6 100644
> --- a/src/test/regress/sql/jsonb_sqljson.sql
> +++ b/src/test/regress/sql/jsonb_sqljson.sql
> @@ -617,7 +617,7 @@ SELECT * FROM JSON_TABLE(
>  SELECT * FROM JSON_TABLE(
>         jsonb 'null', '$[*]' AS p0
>         COLUMNS (
> -               NESTED PATH '$' AS p1 COLUMNS (
> +               NESTED '$' AS p1 COLUMNS (
>                         NESTED PATH '$' AS p11 COLUMNS ( foo int ),
>                         NESTED PATH '$' AS p12 COLUMNS ( bar int )
>                 ),

Fixed the test case like that in the attached.

> Having done that, AFAICS there are two possible fixes for the grammar.
> One is to keep the idea of assigning precedence explicitly to these
> keywords, but do something less hackish -- we can put NESTED together
> with UNBOUNDED, and classify PATH in the IDENT group.  This requires no
> further changes.  This would make NESTED PATH follow the same rationale
> as UNBOUNDED FOLLOWING / UNBOUNDED PRECEDING.  Here's is a preliminary
> patch for that (the large comment above needs to be updated.)
>
> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index c15fcf2eb2..1493ac7580 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -887,9 +887,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>   * json_predicate_type_constraint and json_key_uniqueness_constraint_opt
>   * productions (see comments there).
>   */
> -%nonassoc      UNBOUNDED               /* ideally would have same precedence as IDENT */
> +%nonassoc      UNBOUNDED NESTED                /* ideally would have same precedence as IDENT */
>  %nonassoc      IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
> -                       SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT
> +                       SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT PATH
>  %left          Op OPERATOR             /* multi-character ops and user-defined operators */
>  %left          '+' '-'
>  %left          '*' '/' '%'
> @@ -911,8 +911,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>   */
>  %left          JOIN CROSS LEFT FULL RIGHT INNER_P NATURAL
>
> -%nonassoc      NESTED
> -%left          PATH
>  %%
>
>  /*
>
>
> The other thing we can do is use the two-token lookahead trick, by
> declaring
> %token NESTED_LA
> and using the parser.c code to replace NESTED with NESTED_LA when it is
> followed by PATH.  This doesn't require assigning precedence to
> anything.  We do need to expand the two rules that have "NESTED
> opt_path Sconst" to each be two rules, one for "NESTED_LA PATH Sconst"
> and another for "NESTED Sconst".  So the opt_path production goes away.
> This preliminary patch does that. (I did not touch the ecpg grammar, but
> it needs an update too.)
>
> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index c15fcf2eb2..8e4c1d4ebe 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -817,7 +817,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>   * FORMAT_LA, NULLS_LA, WITH_LA, and WITHOUT_LA are needed to make the grammar
>   * LALR(1).
>   */
> -%token         FORMAT_LA NOT_LA NULLS_LA WITH_LA WITHOUT_LA
> +%token         FORMAT_LA NESTED_LA NOT_LA NULLS_LA WITH_LA WITHOUT_LA
>
>  /*
>   * The grammar likewise thinks these tokens are keywords, but they are never
> @@ -911,8 +911,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
>   */
>  %left          JOIN CROSS LEFT FULL RIGHT INNER_P NATURAL
>
> -%nonassoc      NESTED
> -%left          PATH
>  %%
>
>  /*
> @@ -16771,7 +16769,7 @@ json_table_column_definition:
>                                         n->location = @1;
>                                         $$ = (Node *) n;
>                                 }
> -                       | NESTED path_opt Sconst
> +                       | NESTED_LA PATH Sconst
>                                 COLUMNS '('     json_table_column_definition_list ')'
>                                 {
>                                         JsonTableColumn *n = makeNode(JsonTableColumn);
> @@ -16783,7 +16781,19 @@ json_table_column_definition:
>                                         n->location = @1;
>                                         $$ = (Node *) n;
>                                 }
> -                       | NESTED path_opt Sconst AS name
> +                       | NESTED Sconst
> +                               COLUMNS '('     json_table_column_definition_list ')'
> +                               {
> +                                       JsonTableColumn *n = makeNode(JsonTableColumn);
> +
> +                                       n->coltype = JTC_NESTED;
> +                                       n->pathspec = $2;
> +                                       n->pathname = NULL;
> +                                       n->columns = $5;
> +                                       n->location = @1;
> +                                       $$ = (Node *) n;
> +                               }
> +                       | NESTED_LA PATH Sconst AS name
>                                 COLUMNS '('     json_table_column_definition_list ')'
>                                 {
>                                         JsonTableColumn *n = makeNode(JsonTableColumn);
> @@ -16795,6 +16805,19 @@ json_table_column_definition:
>                                         n->location = @1;
>                                         $$ = (Node *) n;
>                                 }
> +                       | NESTED Sconst AS name
> +                               COLUMNS '('     json_table_column_definition_list ')'
> +                               {
> +                                       JsonTableColumn *n = makeNode(JsonTableColumn);
> +
> +                                       n->coltype = JTC_NESTED;
> +                                       n->pathspec = $2;
> +                                       n->pathname = $4;
> +                                       n->columns = $7;
> +                                       n->location = @1;
> +                                       $$ = (Node *) n;
> +                               }
> +
>                 ;
>
>  json_table_column_path_specification_clause_opt:
> @@ -16802,11 +16825,6 @@ json_table_column_path_specification_clause_opt:
>                         | /* EMPTY */                                                   { $$ = NULL; }
>                 ;
>
> -path_opt:
> -                       PATH                                                            { }
> -                       | /* EMPTY */                                           { }
> -               ;
> -
>  json_table_plan_clause_opt:
>                         PLAN '(' json_table_plan ')'                    { $$ = $3; }
>                         | PLAN DEFAULT '(' json_table_default_plan_choices ')'
> diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
> index e17c310cc1..e3092f2c3e 100644
> --- a/src/backend/parser/parser.c
> +++ b/src/backend/parser/parser.c
> @@ -138,6 +138,7 @@ base_yylex(YYSTYPE *lvalp, YYLTYPE *llocp, core_yyscan_t yyscanner)
>         switch (cur_token)
>         {
>                 case FORMAT:
> +               case NESTED:
>                         cur_token_length = 6;
>                         break;
>                 case NOT:
> @@ -204,6 +205,16 @@ base_yylex(YYSTYPE *lvalp, YYLTYPE *llocp, core_yyscan_t yyscanner)
>                         }
>                         break;
>
> +               case NESTED:
> +                       /* Replace NESTED by NESTED_LA if it's followed by PATH */
> +                       switch (next_token)
> +                       {
> +                               case PATH:
> +                                       cur_token = NESTED_LA;
> +                                       break;
> +                       }
> +                       break;
> +
>                 case NOT:
>                         /* Replace NOT by NOT_LA if it's followed by BETWEEN, IN, etc */
>                         switch (next_token)
>
>
> I don't know which of the two "fixes" is less bad.

I think I'm inclined toward adapting the LA-token fix (attached 0005),
because we've done that before with SQL/JSON constructors patch.
Also, if I understand the concerns that Tom mentioned at [1]
correctly, maybe we'd be better off not assigning precedence to
symbols as much as possible, so there's that too against the approach
#1.

Also I've attached 0006 to add news tests under ECPG for the SQL/JSON
query functions, which I haven't done so far but realized after you
mentioned ECPG.  It also includes the ECPG variant of the LA-token
fix.  I'll eventually merge it into 0003 and 0004 after expanding the
test cases some more.  I do wonder what kinds of tests we normally add
to ECPG suite but not others?

Finally, I also fixed a couple of silly mistakes in 0003 around
transformJsonBehavior() and some further assorted tightening in the ON
ERROR / EMPTY expression coercion handling code.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com