Re: SQL/JSON: functions

On 1/12/22 15:49, Andrew Dunstan wrote:
> 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,
>
>

Here's a patch set with all of these except the last fixed.

There are a couple of things that bother me:

1. This involves a sizeable addition to the grammar, with something over
20 new keywords, including "string" as a  TYPE_FUNC_NAME_KEYWORD. I
guess we can't control what the SQL Standards Committee does, so if we
want to implement this we just need to suck it up. But it's annoying
that they are so profligate with grammar symbols.

2. The new GUC "sql_json" is a bit of a worry. I understand what it's
trying to do, but I'm trying to convince myself it's not going to be a
fruitful source of error reports, especially if people switch it in the
middle of a session. Maybe it should be an initdb option instead of a GUC?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com