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/