Re: remaining sql/json patches

On Fri, Dec 22, 2023 at 9:01 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> Hi
>
> + /* FALLTHROUGH */
> + case JTC_EXISTS:
> + case JTC_FORMATTED:
> + {
> + Node *je;
> + CaseTestExpr *param = makeNode(CaseTestExpr);
> +
> + param->collation = InvalidOid;
> + param->typeId = cxt->contextItemTypid;
> + param->typeMod = -1;
> +
> + if (rawc->wrapper != JSW_NONE &&
> + rawc->quotes != JS_QUOTES_UNSPEC)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("cannot use WITH WRAPPER clause for formatted colunmns"
> + " without also specifying OMIT/KEEP QUOTES"),
> + parser_errposition(pstate, rawc->location)));
>
> typo, should be "formatted columns".
> I suspect people will be confused with the meaning of "formatted column".
> maybe we can replace this part:"cannot use WITH WRAPPER clause for
> formatted column"
> to
> "SQL/JSON WITH WRAPPER behavior must not be specified when FORMAT
> clause is used"
>
> SELECT * FROM JSON_TABLE(jsonb '"world"', '$' COLUMNS (item text
> FORMAT JSON PATH '$' with wrapper KEEP QUOTES));
> ERROR: cannot use WITH WRAPPER clause for formatted colunmns without
> also specifying OMIT/KEEP QUOTES
> LINE 1: ...T * FROM JSON_TABLE(jsonb '"world"', '$' COLUMNS (item text ...
> ^
> this error is misleading, since now I am using WITH WRAPPER clause for
> formatted columns and specified KEEP QUOTES.
>

Hi. still based on v33.
JSON_TABLE:
I also refactor parse_jsontable.c error reporting, now the error
message will be consistent with json_query.
now you can specify wrapper freely as long as you don't specify
wrapper and quote at the same time.
overall, json_table behavior is more consistent with json_query and json_value.
I also added some tests.

+void
+ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op,
+ ExprContext *econtext)
+{
+ JsonCoercion *coercion = op->d.jsonexpr_coercion.coercion;
+ ErrorSaveContext *escontext = op->d.jsonexpr_coercion.escontext;
+ Datum res = *op->resvalue;
+ bool resnull = *op->resnull;
+
+ if (coercion->via_populate)
+ {
+ void *cache = op->d.jsonexpr_coercion.json_populate_type_cache;
+
+ *op->resvalue = json_populate_type(res, JSONBOID,
+ coercion->targettype,
+ coercion->targettypmod,
+ &cache,
+ econtext->ecxt_per_query_memory,
+ op->resnull, (Node *) escontext);
+ }
+ else if (coercion->via_io)
+ {
+ FmgrInfo *input_finfo = op->d.jsonexpr_coercion.input_finfo;
+ Oid typioparam = op->d.jsonexpr_coercion.typioparam;
+ char *val_string = resnull ? NULL :
+ JsonbUnquote(DatumGetJsonbP(res));
+
+ (void) InputFunctionCallSafe(input_finfo, val_string, typioparam,
+ coercion->targettypmod,
+ (Node *) escontext,
+ op->resvalue);
+ }
via_populate, via_io should be mutually exclusive.
your patch, in some cases, both (coercion->via_io) and
(coercion->via_populate) are true.
(we can use elog find out).
I refactor coerceJsonFuncExprOutput, so now it will be mutually exclusive.
I also add asserts to it.

By default, json_query keeps quotes, json_value omit quotes.
However, json_table will be transformed to json_value or json_query
based on certain criteria,
that means we need to explicitly set the JsonExpr->omit_quotes in the
function transformJsonFuncExpr
for case JSON_QUERY_OP and JSON_VALUE_OP.

We need to know the QUOTE behavior in the function ExecEvalJsonCoercion.
Because for ExecEvalJsonCoercion, the coercion datum source can be a
scalar string item,
scalar items means RETURNING clause is dependent on QUOTE behavior.
keep quotes, omit quotes the results are different.
consider
JSON_QUERY(jsonb'{"rec": "[1,2]"}', '$.rec' returning int4range omit quotes);
and
JSON_QUERY(jsonb'{"rec": "[1,2]"}', '$.rec' returning int4range omit quotes);

to make sure ExecEvalJsonCoercion can distinguish keep and omit quotes,
I added a bool keep_quotes to struct JsonCoercion.
(maybe there is a more simple way, so far, that's what I come up with).
the keep_quotes value will be settled in the function transformJsonFuncExpr.
After refactoring, in ExecEvalJsonCoercion, keep_quotes is true then
call JsonbToCString, else call JsonbUnquote.

example:
SELECT JSON_QUERY(jsonb'{"rec": "{1,2,3}"}', '$.rec' returning int[]
omit quotes);
without my changes, return NULL
with my changes:
{1,2,3}

JSON_VALUE:
main changes:
--- a/src/test/regress/expected/jsonb_sqljson.out
+++ b/src/test/regress/expected/jsonb_sqljson.out
@@ -301,7 +301,11 @@ SELECT JSON_VALUE(jsonb '"2017-02-20"', '$'
RETURNING date) + 9;
-- Test NULL checks execution in domain types
CREATE DOMAIN sqljsonb_int_not_null AS int NOT NULL;
SELECT JSON_VALUE(jsonb 'null', '$' RETURNING sqljsonb_int_not_null);
-ERROR: domain sqljsonb_int_not_null does not allow null values
+ json_value
+------------
+
+(1 row)
+
I think the change is correct, given `SELECT JSON_VALUE(jsonb 'null',
'$' RETURNING int4range);` returns NULL.

I also attached a test.sql, without_patch.out (apply v33 only),
with_patch.out (my changes based on v33).
So you can see the difference after applying the patch, in case, my
wording is not clear.