Re: remaining sql/json patches

Hi,

Thanks for the reviews. Replying to all emails here.

On Thu, Nov 23, 2023 at 3:55 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> minor issue.
> maybe you can add the following after
> /src/test/regress/sql/jsonb_sqljson.sql: 127.
> Test coverage for ExecPrepareJsonItemCoercion function.
>
> SELECT JSON_VALUE(jsonb 'null', '$ts' PASSING date '2018-02-21
> 12:34:56 +10' AS ts returning date);
> SELECT JSON_VALUE(jsonb 'null', '$ts' PASSING time '2018-02-21
> 12:34:56 +10' AS ts returning time);
> SELECT JSON_VALUE(jsonb 'null', '$ts' PASSING timetz '2018-02-21
> 12:34:56 +10' AS ts returning timetz);
> SELECT JSON_VALUE(jsonb 'null', '$ts' PASSING timestamp '2018-02-21
> 12:34:56 +10' AS ts returning timestamp);

Added, though I decided to not include the function name in the
comment and rather reworded the nearby comment a bit.

On Thu, Nov 23, 2023 at 7:47 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> +/*
> + * Evaluate or return the step address to evaluate a coercion of a JSON item
> + * to the target type. The former if the coercion must be done right away by
> + * calling the target type's input function, and for some types, by calling
> + * json_populate_type().
> + *
> + * Returns the step address to be performed next.
> + */
> +void
> +ExecEvalJsonCoercionViaPopulateOrIO(ExprState *state, ExprEvalStep *op,
> + ExprContext *econtext)
>
> the comment seems not right? it does return anything. it did the evaluation.

Fixed the comment. Actually, I've also restored the old name of the
function because of reworking coercion machinery to use a JsonCoercion
node only for cases where the coercion is performed using I/O or
json_populdate_type().

> some logic in ExecEvalJsonCoercionViaPopulateOrIO, like if
> (SOFT_ERROR_OCCURRED(escontext_p)) and if
> (!InputFunctionCallSafe){...}, seems validated twice,
> ExecEvalJsonCoercionFinish also did it. I uncommented the following
> part, and still passed the test.
> /src/backend/executor/execExprInterp.c
> 4452: // if (SOFT_ERROR_OCCURRED(escontext_p))
> 4453: // {
> 4454: // post_eval->error.value = BoolGetDatum(true);
> 4455: // *op->resvalue = (Datum) 0;
> 4456: // *op->resnull = true;
> 4457: // }
>
> 4470: // post_eval->error.value = BoolGetDatum(true);
> 4471: // *op->resnull = true;
> 4472: // *op->resvalue = (Datum) 0;
> 4473: return;

Yes, you're right. ExecEvalJsonCoercionFinish()'s check for
soft-error suffices.

> Correct me if I'm wrong.
> like in "empty array on empty empty object on error", the "empty
> array" refers to constant literal '[]' the assumed data type is jsonb,
> the "empty object" refers to const literal '{}', the assumed data type
> is jsonb.

That's correct.

> --these two queries will fail very early, before ExecEvalJsonExprPath.
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.a' RETURNING int4range
> default '[1.1,2]' on error);
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.a' RETURNING int4range
> default '[1.1,2]' on empty);

They fail early because the user-specified DEFAULT [ON ERROR/EMPTY]
expression is coerced at parse time.

> -----these four will fail later, and will call
> ExecEvalJsonCoercionViaPopulateOrIO twice.
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> object on empty empty object on error);
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> array on empty empty array on error);
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> array on empty empty object on error);
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> object on empty empty array on error);

With the latest version, you'll now get the following errors:

ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty obje...
^
ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty arra...
^
ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty arra...
^
ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty obje...

> -----however these four will not fail.
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> object on error);
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> array on error);
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> array on empty);
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> object on empty);
> should the last four query fail or just return null?

You'll get the following with the latest version:

ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty obje...
^
ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty arra...
^
ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty arra...
^
ERROR: cannot cast behavior expression of type jsonb to int4range
LINE 1: ...RY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty obje...

On Fri, Nov 24, 2023 at 5:41 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> + /*
> + * Set information for RETURNING type's input function used by
> + * ExecEvalJsonExprCoercion().
> + */
> "ExecEvalJsonExprCoercion" comment is wrong?

Comment removed in the latest version.

> + /*
> + * Step to jump to the EEOP_JSONEXPR_FINISH step skipping over item
> + * coercion steps that will be added below, if any.
> + */
> "EEOP_JSONEXPR_FINISH" comment is wrong?

Not wrong though the wording is misleading. It's describing what will
happen at runtime -- jump after performing result_coercion to skip
over any steps that might be present between the last of the
result_coercion steps and the EEOP_JSONEXPR_FINISH step. You can see
the code that follows is adding steps for JSON_VALUE "item" coercions,
which will be skipped by performing that jump.

> seems on error, on empty behavior have some issues. The following are
> tests for json_value.
> select json_value(jsonb '{"a":[123.45,1]}', '$.z' returning text
> error on error);
> select json_value(jsonb '{"a":[123.45,1]}', '$.z' returning text
> error on empty); ---imho, this should fail?
> select json_value(jsonb '{"a":[123.45,1]}', '$.z' returning text
> error on empty error on error);

Yes, I agree there are issues. I think these all should give an
error. So, the no-match scenario (empty=true) should give an error
both when ERROR ON EMPTY is specified and also if only ERROR ON ERROR
is specified. With the current code, ON ERROR basically overrides ON
EMPTY clause which seems wrong.

With the latest patch, you'll get the following:

select json_value(jsonb '{"a":[123.45,1]}', '$.z' returning text
error on error);
ERROR: no SQL/JSON item

select json_value(jsonb '{"a":[123.45,1]}', '$.z' returning text
error on empty); ---imho, this should fail?
ERROR: no SQL/JSON item

select json_value(jsonb '{"a":[123.45,1]}', '$.z' returning text
error on empty error on error);
ERROR: no SQL/JSON item

> I did some minor refactoring, please see the attached.
> In transformJsonFuncExpr, only (jsexpr->result_coercion) is not null
> then do InitJsonItemCoercions.

Makes sense.

> The ExecInitJsonExpr ending part is for Adjust EEOP_JUMP steps. so I
> moved "Set information for RETURNING type" inside
> if (jexpr->result_coercion || jexpr->omit_quotes).
> there are two if (jexpr->item_coercions). so I combined them together.

This code has moved to a different place with the latest patch,
wherein I've redesigned the io/populate-based coercions.

On Tue, Nov 28, 2023 at 11:01 AM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> On Thu, Nov 23, 2023 at 6:46 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> >
> > -----however these four will not fail.
> > SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> > object on error);
> > SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> > array on error);
> > SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> > array on empty);
> > SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> > object on empty);
> >
> > should the last four query fail or just return null?
>
> I refactored making the above four queries fail.
> SELECT JSON_QUERY(jsonb '{"a":[3,4]}', '$.z' RETURNING int4range empty
> object on error);
> The new error is: ERROR: cannot cast DEFAULT expression of type jsonb
> to int4range.
>
> also make the following query fail, which is as expected, imho.
> select json_value(jsonb '{"a":[123.45,1]}', '$.z' returning text
> error on empty);

Agreed. I've incorporated your suggestions into the latest patch
though not using the exact code that you shared.

Attached please find the latest patches. Other than the points
mentioned above, I've made substantial changes to how JsonBehavior and
JsonCoercion nodes work.

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

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