Horton
Menu

Re: SQL/JSON: functions

Andrew Dunstan <andrew@dunslane.net>

View thread
From: Andrew Dunstan <andrew@dunslane.net>
To: Himanshu Upadhyaya <upadhyaya.himanshu@gmail.com>
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>, PostgreSQL-development <pgsql-hackers@postgresql.org>, Dmitry Dolgov <9erthalion6@gmail.com>, Oleg Bartunov <obartunov@postgrespro.ru>, Erik Rijkers <er@xs4all.nl>
Date: 2022-02-01T19:11:32Z
Lists: pgsql-hackers

Attachments

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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. JSON_TABLE: Add support for NESTED paths and columns

  2. Add basic JSON_TABLE() functionality

  3. Add SQL/JSON query functions

  4. Add soft error handling to some expression nodes

  5. Adjust populate_record_field() to handle errors softly

  6. Refactor code used by jsonpath executor to fetch variables

  7. Add more SQL/JSON constructor functions

  8. SQL/JSON: support the IS JSON predicate

  9. SQL/JSON: add standard JSON constructor functions

  10. Improve readability of SQL/JSON documentation.

  11. Documentation for SQL/JSON features

  12. RETURNING clause for JSON() and JSON_SCALAR()

  13. SQL JSON functions

  14. SQL/JSON query functions

  15. IS JSON predicate

  16. SQL/JSON constructors

  17. Common SQL/JSON clauses

  18. Doc: remove obsolete entries in table of BRIN strategy numbers.

  19. Doc: standardize markup a bit more.

  20. Make pg_regress.c unset PGDATABASE during make installcheck.

  21. Fix handling of GENERATED columns in CREATE TABLE LIKE INCLUDING DEFAULTS.