Re: remaining sql/json patches

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 )
 		),


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.  Like Amit, I was not
able to find a solution to the problem by merely attaching precedences
to rules.  (I did not try to mess with the precedence of
unreserved_keyword, because I'm pretty sure that would not be a good
solution even if I could make it work.)

[1] https://postgr.es/m/CADT4RqBPdbsZW7HS1jJP319TMRHs1hzUiP=iRJYR6UqgHCrgNQ@mail.gmail.com

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/