From cbcd9c47926fade076fd474e3f43fa36cf79d0dc Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Tue, 9 Apr 2024 20:31:30 +0900 Subject: [PATCH v4] SQL/JSON: Miscellaneous fixes and improvements This addresses some post-commit review comments for commits 6185c973, de3600452, and 9425c596a0, with the following changes: * Improve some SQL/JSON query function error messages by mentioning the column name when available, such as when they are invoked as part of evaluating JSON_TABLE() columns. To do so, a new field column_name is added to both JsonFuncExpr and JsonExpr that is only populated when creating those nodes when transforming the JSON_TABLE() columns' parser nodes. * Reword SQL/JSON query function error messages for clarity. * Fix JSON_TABLE() syntax documentation to use the term "path_expression" for JSON path expressions instead of "json_path_specification" to be consistent with the rest of the SQL/JSON functions. * Fix a typo in the example code in JSON_TABLE() documentation. * Fix some newly added comments in jsonpath.h. * In JsonPathQuery(), add missing cast to int before printing an enum value. Reported-by: Jian He Suggested-by: Jian He Discussion: https://postgr.es/m/CACJufxG_e0QLCgaELrr2ZNz7AxPeGCNKAORe3fHtFCQLsH4J4Q@mail.gmail.com --- doc/src/sgml/func.sgml | 9 ++-- src/backend/executor/execExprInterp.c | 42 ++++++++++------- src/backend/parser/parse_expr.c | 1 + src/backend/parser/parse_jsontable.c | 9 ++-- src/backend/utils/adt/jsonpath_exec.c | 47 ++++++++++++++----- src/include/nodes/parsenodes.h | 2 + src/include/nodes/primnodes.h | 3 ++ src/include/utils/jsonpath.h | 14 +++--- .../regress/expected/sqljson_jsontable.out | 10 ++-- .../regress/expected/sqljson_queryfuncs.out | 10 ++-- src/test/regress/sql/sqljson_jsontable.sql | 3 ++ 11 files changed, 94 insertions(+), 56 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 8dfb42ad4d..92a0f49e6a 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -18942,7 +18942,7 @@ where json_table_column is: { ERROR | NULL | EMPTY { ARRAY | OBJECT } | DEFAULT expression } ON ERROR | name type EXISTS PATH path_expression { ERROR | TRUE | FALSE | UNKNOWN } ON ERROR - | NESTED PATH json_path_specification AS json_path_name COLUMNS ( json_table_column , ... ) + | NESTED PATH path_expression AS json_path_name COLUMNS ( json_table_column , ... ) @@ -19083,7 +19083,7 @@ where json_table_column is: - NESTED PATH json_path_specification AS json_path_name + NESTED PATH path_expression AS json_path_name COLUMNS ( json_table_column , ... ) @@ -19315,8 +19315,9 @@ SELECT * FROM JSON_TABLE ( "books": [{"name": "Mystery", "authors": [{"name": "Brown Dan"}]}, {"name": "Wonder", "authors": [{"name": "Jun Murakami"}, {"name":"Craig Doe"}]}] -}}'::json, '$.favs[*]' -COLUMNS (user_id FOR ORDINALITY, +}}'::json, '$.favorites[*]' +COLUMNS ( + user_id FOR ORDINALITY, NESTED '$.movies[*]' COLUMNS ( movie_id FOR ORDINALITY, diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 41af28cb1e..580c897144 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -4312,7 +4312,8 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, case JSON_QUERY_OP: *op->resvalue = JsonPathQuery(item, path, jsexpr->wrapper, &empty, !throw_error ? &error : NULL, - jsestate->args); + jsestate->args, + jsexpr->column_name); *op->resnull = (DatumGetPointer(*op->resvalue) == NULL); @@ -4337,7 +4338,8 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, { JsonbValue *jbv = JsonPathValue(item, path, &empty, !throw_error ? &error : NULL, - jsestate->args); + jsestate->args, + jsexpr->column_name); if (jbv == NULL) { @@ -4409,28 +4411,32 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, { if (jsexpr->on_empty) { - if (jsexpr->on_empty->btype == JSON_BEHAVIOR_ERROR) - ereport(ERROR, - errcode(ERRCODE_NO_SQL_JSON_ITEM), - errmsg("no SQL/JSON item")); - else + if (jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR) + { jsestate->empty.value = BoolGetDatum(true); + Assert(jsestate->jump_empty >= 0); + return jsestate->jump_empty; + } + } + else if (jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR) + { + jsestate->error.value = BoolGetDatum(true); - Assert(jsestate->jump_empty >= 0); - return jsestate->jump_empty; + *op->resvalue = (Datum) 0; + *op->resnull = true; + Assert(!throw_error && jsestate->jump_error >= 0); + return jsestate->jump_error; } - else if (jsexpr->on_error->btype == JSON_BEHAVIOR_ERROR) + + if (jsexpr->column_name) ereport(ERROR, errcode(ERRCODE_NO_SQL_JSON_ITEM), - errmsg("no SQL/JSON item")); + errmsg("no SQL/JSON item found for column \"%s\"", + jsexpr->column_name)); else - jsestate->error.value = BoolGetDatum(true); - - *op->resvalue = (Datum) 0; - *op->resnull = true; - - Assert(!throw_error && jsestate->jump_error >= 0); - return jsestate->jump_error; + ereport(ERROR, + errcode(ERRCODE_NO_SQL_JSON_ITEM), + errmsg("no SQL/JSON item")); } /* diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c index 4c98d7a046..34ac17868b 100644 --- a/src/backend/parser/parse_expr.c +++ b/src/backend/parser/parse_expr.c @@ -4311,6 +4311,7 @@ transformJsonFuncExpr(ParseState *pstate, JsonFuncExpr *func) jsexpr = makeNode(JsonExpr); jsexpr->location = func->location; jsexpr->op = func->op; + jsexpr->column_name = func->column_name; /* * jsonpath machinery can only handle jsonb documents, so coerce the input diff --git a/src/backend/parser/parse_jsontable.c b/src/backend/parser/parse_jsontable.c index 99d3101f6b..70b00a45f0 100644 --- a/src/backend/parser/parse_jsontable.c +++ b/src/backend/parser/parse_jsontable.c @@ -402,12 +402,6 @@ transformJsonTableColumn(JsonTableColumn *jtc, Node *contextItemExpr, Node *pathspec; JsonFuncExpr *jfexpr = makeNode(JsonFuncExpr); - /* - * XXX consider inventing JSON_TABLE_VALUE_OP, etc. and pass the column - * name via JsonExpr so that JsonPathValue(), etc. can provide error - * message tailored to JSON_TABLE(), such as by mentioning the column - * names in the message. - */ if (jtc->coltype == JTC_REGULAR) jfexpr->op = JSON_VALUE_OP; else if (jtc->coltype == JTC_EXISTS) @@ -415,6 +409,9 @@ transformJsonTableColumn(JsonTableColumn *jtc, Node *contextItemExpr, else jfexpr->op = JSON_QUERY_OP; + /* Pass the column name so any runtime JsonExpr errors can print it. */ + jfexpr->column_name = pstrdup(jtc->name); + jfexpr->context_item = makeJsonValueExpr((Expr *) contextItemExpr, NULL, makeJsonFormat(JS_FORMAT_DEFAULT, JS_ENC_DEFAULT, diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c index 103572ed93..4daf1a68d9 100644 --- a/src/backend/utils/adt/jsonpath_exec.c +++ b/src/backend/utils/adt/jsonpath_exec.c @@ -3899,7 +3899,8 @@ JsonPathExists(Datum jb, JsonPath *jp, bool *error, List *vars) */ Datum JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, - bool *error, List *vars) + bool *error, List *vars, + const char *column_name) { JsonbValue *singleton; bool wrap; @@ -3934,7 +3935,7 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, JsonContainerIsScalar(singleton->val.binary.data)); else { - elog(ERROR, "unrecognized json wrapper %d", wrapper); + elog(ERROR, "unrecognized json wrapper %d", (int) wrapper); wrap = false; } @@ -3950,10 +3951,17 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, return (Datum) 0; } - ereport(ERROR, - (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM), - errmsg("JSON path expression in JSON_QUERY should return singleton item without wrapper"), - errhint("Use WITH WRAPPER clause to wrap SQL/JSON item sequence into array."))); + if (column_name) + ereport(ERROR, + (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM), + errmsg("JSON path expression for column \"%s\" should return single item without wrapper", + column_name), + errhint("Use WITH WRAPPER clause to wrap SQL/JSON items into array."))); + else + ereport(ERROR, + (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM), + errmsg("JSON path expression in JSON_QUERY should return single item without wrapper"), + errhint("Use WITH WRAPPER clause to wrap SQL/JSON items into array."))); } if (singleton) @@ -3970,7 +3978,8 @@ JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, bool *empty, * *error to true. *empty is set to true if no match is found. */ JsonbValue * -JsonPathValue(Datum jb, JsonPath *jp, bool *empty, bool *error, List *vars) +JsonPathValue(Datum jb, JsonPath *jp, bool *empty, bool *error, List *vars, + const char *column_name) { JsonbValue *res; JsonValueList found = {0}; @@ -4006,9 +4015,15 @@ JsonPathValue(Datum jb, JsonPath *jp, bool *empty, bool *error, List *vars) return NULL; } - ereport(ERROR, - (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM), - errmsg("JSON path expression in JSON_VALUE should return singleton scalar item"))); + if (column_name) + ereport(ERROR, + (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM), + errmsg("JSON path expression for column \"%s\" should return single scalar item", + column_name))); + else + ereport(ERROR, + (errcode(ERRCODE_MORE_THAN_ONE_SQL_JSON_ITEM), + errmsg("JSON path expression in JSON_VALUE should return single scalar item"))); } res = JsonValueListHead(&found); @@ -4024,9 +4039,15 @@ JsonPathValue(Datum jb, JsonPath *jp, bool *empty, bool *error, List *vars) return NULL; } - ereport(ERROR, - (errcode(ERRCODE_SQL_JSON_SCALAR_REQUIRED), - errmsg("JSON path expression in JSON_VALUE should return singleton scalar item"))); + if (column_name) + ereport(ERROR, + (errcode(ERRCODE_SQL_JSON_SCALAR_REQUIRED), + errmsg("JSON path expression for column \"%s\" should return single scalar item", + column_name))); + else + ereport(ERROR, + (errcode(ERRCODE_SQL_JSON_SCALAR_REQUIRED), + errmsg("JSON path expression in JSON_VALUE should return single scalar item"))); } if (res->type == jbvNull) diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index f763f790b1..c0ff75e643 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1791,6 +1791,8 @@ typedef struct JsonFuncExpr { NodeTag type; JsonExprOp op; /* expression type */ + char *column_name; /* JSON_TABLE() column name or NULL if this is + * not for a JSON_TABLE() */ JsonValueExpr *context_item; /* context item expression */ Node *pathspec; /* JSON path specification expression */ List *passing; /* list of PASSING clause arguments, if any */ diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index dafe93a4c9..9b662b8dd2 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -1782,6 +1782,9 @@ typedef struct JsonExpr JsonExprOp op; + char *column_name; /* JSON_TABLE() column name or NULL if this is + * not for a JSON_TABLE() */ + /* jsonb-valued expression to query */ Node *formatted_expr; diff --git a/src/include/utils/jsonpath.h b/src/include/utils/jsonpath.h index 4d3964488d..69c180c2e2 100644 --- a/src/include/utils/jsonpath.h +++ b/src/include/utils/jsonpath.h @@ -281,12 +281,9 @@ extern JsonPathParseResult *parsejsonpath(const char *str, int len, extern bool jspConvertRegexFlags(uint32 xflags, int *result, struct Node *escontext); - /* - * Evaluation of jsonpath + * Struct for details about external variables passed into jsonpath executor */ - -/* External variable passed into jsonpath. */ typedef struct JsonPathVariable { char *name; @@ -297,13 +294,16 @@ typedef struct JsonPathVariable } JsonPathVariable; -/* SQL/JSON item */ +/* SQL/JSON query functions */ extern bool JsonPathExists(Datum jb, JsonPath *path, bool *error, List *vars); extern Datum JsonPathQuery(Datum jb, JsonPath *jp, JsonWrapper wrapper, - bool *empty, bool *error, List *vars); + bool *empty, bool *error, List *vars, + const char *column_name); extern JsonbValue *JsonPathValue(Datum jb, JsonPath *jp, bool *empty, - bool *error, List *vars); + bool *error, List *vars, + const char *column_name); +/* For JSON_TABLE() */ extern PGDLLIMPORT const TableFuncRoutine JsonbTableRoutine; #endif diff --git a/src/test/regress/expected/sqljson_jsontable.out b/src/test/regress/expected/sqljson_jsontable.out index a00eec8a6f..d65fa7fe69 100644 --- a/src/test/regress/expected/sqljson_jsontable.out +++ b/src/test/regress/expected/sqljson_jsontable.out @@ -492,11 +492,11 @@ FROM ON true; ERROR: invalid input syntax for type integer: "err" SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int PATH '$.a' ERROR ON EMPTY)) jt; -ERROR: no SQL/JSON item +ERROR: no SQL/JSON item found for column "a" SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int PATH 'strict $.a' ERROR ON ERROR) ERROR ON ERROR) jt; ERROR: jsonpath member accessor can only be applied to an object SELECT * FROM JSON_TABLE(jsonb '1', '$' COLUMNS (a int PATH 'lax $.a' ERROR ON EMPTY) ERROR ON ERROR) jt; -ERROR: no SQL/JSON item +ERROR: no SQL/JSON item found for column "a" SELECT * FROM JSON_TABLE(jsonb '"a"', '$' COLUMNS (a int PATH '$' DEFAULT 1 ON EMPTY DEFAULT 2 ON ERROR)) jt; a --- @@ -637,6 +637,10 @@ SELECT * FROM JSON_TABLE(jsonb '{"a": 123}', '$' || '.' || 'a' COLUMNS (foo int) ERROR: only string constants are supported in JSON_TABLE path specification LINE 1: SELECT * FROM JSON_TABLE(jsonb '{"a": 123}', '$' || '.' || '... ^ +-- JsonPathQuery() error message mentioning column name +SELECT * FROM JSON_TABLE('{"a": [{"b": "1"}, {"b": "2"}]}', '$' COLUMNS (b json path '$.a[*].b' ERROR ON ERROR)); +ERROR: JSON path expression for column "b" should return single item without wrapper +HINT: Use WITH WRAPPER clause to wrap SQL/JSON items into array. -- JSON_TABLE: nested paths -- Duplicate path names SELECT * FROM JSON_TABLE( @@ -849,7 +853,7 @@ SELECT sub.* FROM s, xx int path '$.c', NESTED PATH '$.a.za[1]' columns (NESTED PATH '$.z21[*]' COLUMNS (z21 int path '$?(@ >= $"x")' ERROR ON ERROR)) )) sub; -ERROR: no SQL/JSON item +ERROR: no SQL/JSON item found for column "z21" -- Parent columns xx1, xx appear before NESTED ones SELECT sub.* FROM s, (VALUES (23)) x(x), generate_series(13, 13) y, diff --git a/src/test/regress/expected/sqljson_queryfuncs.out b/src/test/regress/expected/sqljson_queryfuncs.out index 9e86b0da10..6c5f1b3ecd 100644 --- a/src/test/regress/expected/sqljson_queryfuncs.out +++ b/src/test/regress/expected/sqljson_queryfuncs.out @@ -339,7 +339,7 @@ SELECT JSON_VALUE(jsonb '[]', '$'); (1 row) SELECT JSON_VALUE(jsonb '[]', '$' ERROR ON ERROR); -ERROR: JSON path expression in JSON_VALUE should return singleton scalar item +ERROR: JSON path expression in JSON_VALUE should return single scalar item SELECT JSON_VALUE(jsonb '{}', '$'); json_value ------------ @@ -347,7 +347,7 @@ SELECT JSON_VALUE(jsonb '{}', '$'); (1 row) SELECT JSON_VALUE(jsonb '{}', '$' ERROR ON ERROR); -ERROR: JSON path expression in JSON_VALUE should return singleton scalar item +ERROR: JSON path expression in JSON_VALUE should return single scalar item SELECT JSON_VALUE(jsonb '1', '$.a'); json_value ------------ @@ -399,7 +399,7 @@ SELECT JSON_VALUE(jsonb '1', 'lax $.a' DEFAULT '2' ON EMPTY DEFAULT '3' ON ERROR SELECT JSON_VALUE(jsonb '1', 'lax $.a' ERROR ON EMPTY DEFAULT '3' ON ERROR); ERROR: no SQL/JSON item SELECT JSON_VALUE(jsonb '[1,2]', '$[*]' ERROR ON ERROR); -ERROR: JSON path expression in JSON_VALUE should return singleton scalar item +ERROR: JSON path expression in JSON_VALUE should return single scalar item SELECT JSON_VALUE(jsonb '[1,2]', '$[*]' DEFAULT '0' ON ERROR); json_value ------------ @@ -776,8 +776,8 @@ ERROR: no SQL/JSON item SELECT JSON_QUERY(jsonb '[]', '$[*]' ERROR ON ERROR); ERROR: no SQL/JSON item SELECT JSON_QUERY(jsonb '[1,2]', '$[*]' ERROR ON ERROR); -ERROR: JSON path expression in JSON_QUERY should return singleton item without wrapper -HINT: Use WITH WRAPPER clause to wrap SQL/JSON item sequence into array. +ERROR: JSON path expression in JSON_QUERY should return single item without wrapper +HINT: Use WITH WRAPPER clause to wrap SQL/JSON items into array. SELECT JSON_QUERY(jsonb '[1,2]', '$[*]' DEFAULT '"empty"' ON ERROR); json_query ------------ diff --git a/src/test/regress/sql/sqljson_jsontable.sql b/src/test/regress/sql/sqljson_jsontable.sql index 3752ccc446..29c0c6ba52 100644 --- a/src/test/regress/sql/sqljson_jsontable.sql +++ b/src/test/regress/sql/sqljson_jsontable.sql @@ -290,6 +290,9 @@ FROM JSON_TABLE( -- Should fail (not supported) SELECT * FROM JSON_TABLE(jsonb '{"a": 123}', '$' || '.' || 'a' COLUMNS (foo int)); +-- JsonPathQuery() error message mentioning column name +SELECT * FROM JSON_TABLE('{"a": [{"b": "1"}, {"b": "2"}]}', '$' COLUMNS (b json path '$.a[*].b' ERROR ON ERROR)); + -- JSON_TABLE: nested paths -- Duplicate path names -- 2.43.0