Re: remaining sql/json patches

On Sat, Dec 9, 2023 at 2:05 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> Hi.

Thanks for the review.

> function JsonPathExecResult comment needs to be refactored? since it
> changed a lot.

I suppose you meant executeJsonPath()'s comment. I've added a
description of the new callback function arguments.

On Wed, Dec 13, 2023 at 6:59 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> Hi. small issues I found...
>
> typo:
> +-- Test mutabilily od query functions

Fixed.

>
> + default:
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("only datetime, bool, numeric, and text types can be casted
> to jsonpath types")));
>
> transformJsonPassingArgs's function: transformJsonValueExpr will make
> the above code unreached.

It's good to have the ereport to catch errors caused by any future changes.

> also based on the `switch (typid)` cases,
> I guess best message would be
> errmsg("only datetime, bool, numeric, text, json, jsonb types can be
> casted to jsonpath types")));

I've rewritten the message to mention the unsupported type. Maybe the
supported types can go in a DETAIL message. I might do that later.

> + case JSON_QUERY_OP:
> + jsexpr->wrapper = func->wrapper;
> + jsexpr->omit_quotes = (func->quotes == JS_QUOTES_OMIT);
> +
> + if (!OidIsValid(jsexpr->returning->typid))
> + {
> + JsonReturning *ret = jsexpr->returning;
> +
> + ret->typid = JsonFuncExprDefaultReturnType(jsexpr);
> + ret->typmod = -1;
> + }
> + jsexpr->result_coercion = coerceJsonFuncExprOutput(pstate, jsexpr);
>
> I noticed, if (!OidIsValid(jsexpr->returning->typid)) is the true
> function JsonFuncExprDefaultReturnType may be called twice, not sure
> if it's good or not..

If avoiding the double-calling means that we've to add more conditions
in the code, I'm fine with leaving this as-is.

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