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-01-12T20:49:41Z
Lists: pgsql-hackers 
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

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.