Horton
Menu

Re: SQL/JSON: functions

Nikita Glukhov <n.gluhov@postgrespro.ru>

View thread
From: Nikita Glukhov <n.gluhov@postgrespro.ru>
To: Zhihong Yu <zyu@yugabyte.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Cc: Michael Paquier <michael@paquier.xyz>, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>, Justin Pryzby <pryzby@telsasoft.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, Pavel Stehule <pavel.stehule@gmail.com>, Erik Rijkers <er@xs4all.nl>, Alvaro Herrera <alvherre@2ndquadrant.com>, Thomas Munro <thomas.munro@gmail.com>, Andrew Alsup <bluesbreaker@gmail.com>, Andres Freund <andres@anarazel.de>, Dmitry Dolgov <9erthalion6@gmail.com>, Oleg Bartunov <obartunov@postgrespro.ru>, Simon Riggs <simon@2ndquadrant.com>
Date: 2021-01-20T02:49:13Z
Lists: pgsql-hackers

Attachments

Hi, Zhihong.  Thank your for your review.  Attached 52nd version of the
patches rebased onto master and fixed as you suggested.


On 26.12.2020 04:19, Zhihong Yu wrote:
> For 0001-Common-SQL-JSON-clauses-v51.patch :
>
> +       /*  | implementation_defined_JSON_representation_option (BSON, 
> AVRO etc) */
>
> I don't find implementation_defined_JSON_representation_option in the 
> patchset. Maybe rephrase the above as a comment 
> without implementation_defined_JSON_representation_option ?
>
Fixed.


> For getJsonEncodingConst(), should the method error out for the 
> default case of switch (encoding) ?

Added default case with elog(ERROR).


>
> 0002-SQL-JSON-constructors-v51.patch :
>
> +                   Assert(!OidIsValid(collation)); /* result is 
> always an json[b] type */
> an json -> a json

Fixed.


>
> +           /* XXX TEXTOID is default by standard */
> +           returning->typid = JSONOID;
>
> Comment doesn't seem to match the assignment.

Comment corrected.

> For json_object_agg_transfn :
>
> +       if (out->len > 2)
> +           appendStringInfoString(out, ", ");
>
> Why length needs to be at least 3 (instead of 2) ?

2 is a length of starting string "{ ".  len > 2 means that we have already
outputted some fields and we need to output comma delimiter.



On 26.12.2020 22:12, Zhihong Yu wrote:
> Hi,
> For ExecEvalJsonExprSubtrans(), if you check !subtrans first,
>
> +       /* No need to use subtransactions. */
> +       return func(op, econtext, res, resnull, p, error);
>
> The return statement would allow omitting the else keyword and 
> left-indent the code in the current if block.

"If" statement was refactored as you suggest.


> For ExecEvalJsonExpr()
>
> +           *resnull = !DatumGetPointer(res);
> +           if (error && *error)
> +               return (Datum) 0;
>
> Suppose *resnull is false and *error is true, 0 would be returned 
> with *resnull as false. Should the *resnull be consistent with the 
> actual return value ?

Now *resnull is set to false in case of error.


> For ExecEvalJson() :
>
> +       Assert(*op->resnull);
> +       *op->resnull = true;
>
> I am not sure of the purpose for the assignment since *op->resnull 
> should be true by the assertion.

Assignment was removed.

> For raw_expression_tree_walker :
>
> +               if (walker(jfe->on_empty, context))
> +                   return true;
>
> Should the if condition include jfe->on_empty prior to walking ?
>
Yes, jfe->on_empty like jfe->on_error can be NULL, and NULL check here is
a walker's responsibility.  But in expression_tree_walker() there is a check
for jfe->on_empty, because only on_empty (not jfe->on_error) can be NULL, and
we are calling walker() on jfe->on_empty->default_expr, not on jfe->on_empty.


>
> nit: for contain_mutable_functions_walker, if !IsA(jexpr->path_spec, 
> Const) is checked first (and return), the current if block can be left 
> indented.

Code was refactored as you suggested.


> For JsonPathDatatypeStatus,
>
> +   jpdsDateTime,               /* unknown datetime type */
>
> Should the enum be named jpdsUnknownDateTime so that its meaning is 
> clear to people reading the code ?

jpdsDateTime was renamed to jpdsUnknownDateTime.


>
> For get_json_behavior(), I wonder if mapping from behavior->btype to 
> the string form would shorten the body of switch statement.
> e.g.
> char* map[] = {
>   " NULL",
>   " ERROR",
>   " EMPTY",
> ...
> };
>
"Switch" statement was replaced with array lookup.


-- 
Nikita Glukhov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

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.