On 12/9/21 09:04, Himanshu Upadhyaya wrote:
>
>
>
> Few comments For 0002-SQL-JSON-constructors-v59.patch:
> 1)
> + if (IsA(node, JsonConstructorExpr))
> + {
> + JsonConstructorExpr *ctor = (JsonConstructorExpr *) node;
> + ListCell *lc;
> + bool is_jsonb =
> + ctor->returning->format->format ==
> JS_FORMAT_JSONB;
> +
> + /* Check argument_type => json[b] conversions */
> + foreach(lc, ctor->args)
> + {
> + Oid typid =
> exprType(lfirst(lc));
> +
> + if (is_jsonb ?
> + !to_jsonb_is_immutable(typid) :
> + !to_json_is_immutable(typid))
> + return true;
> + }
> +
> + /* Check all subnodes */
> + }
> can have ctor as const pointer?
I guess we could, although I'm not sure it's an important advance.
>
> 2)
> +typedef struct JsonFormat
> +{
> + NodeTag type;
> + JsonFormatType format; /* format type */
> + JsonEncoding encoding; /* JSON encoding */
> + int location; /* token
> location, or -1 if unknown */
> +} JsonFormat;
>
> I think it will be good if we can have a JsonformatType(defined in
> patch 0001-Common-SQL-JSON-clauses-v59.patch) member named as
> format_type or formatType instead of format?
> There are places in the patch where we access it as "if
> (format->format == JS_FORMAT_DEFAULT)". "format->format" looks little
> difficult to understand.
> "format->format_type == JS_FORMAT_DEFAULT" will be easy to follow.
That's a fair criticism.
>
> 3)
> + if (have_jsonb)
> + {
> + returning->typid = JSONBOID;
> + returning->format->format = JS_FORMAT_JSONB;
> + }
> + else if (have_json)
> + {
> + returning->typid = JSONOID;
> + returning->format->format = JS_FORMAT_JSON;
> + }
> + else
> + {
> + /* XXX TEXT is default by the standard, but we
> return JSON */
> + returning->typid = JSONOID;
> + returning->format->format = JS_FORMAT_JSON;
> + }
>
> why we need a separate "else if (have_json)" statement in the below
> code, "else" is also doing the same thing?
I imagine it's more or less a placeholder in case we want to do
something else in the default case. But I agree it's confusing.
>
> 4)
> -test: json jsonb json_encoding jsonpath jsonpath_encoding jsonb_jsonpath
> +test: json jsonb json_encoding jsonpath jsonpath_encoding
> jsonb_jsonpath sqljson
>
> can we rename sqljson sql test file to json_constructor?
Not really - the later patches in the series add to it, so it ends up
being more than just constructor tests.
Thanks for reviewing,
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
