public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nikita Glukhov <[email protected]>
To: Zhihong Yu <[email protected]>
To: PostgreSQL-development <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Alexander Korotkov <[email protected]>
Cc: Pavel Stehule <[email protected]>
Cc: Erik Rijkers <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Andrew Alsup <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Dmitry Dolgov <[email protected]>
Cc: Oleg Bartunov <[email protected]>
Cc: Simon Riggs <[email protected]>
Subject: Re: SQL/JSON: functions
Date: Wed, 20 Jan 2021 05:49:13 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALNJ-vSmJbaoUm7nuyV65UdeYTee2UxqG49CZ7CjR_rEB5ZdJg@mail.gmail.com>
References: <[email protected]>
<CAFj8pRA6T1=39PaHH3zL=x4_xp7VSUa+16v3fEgDTzCOoaFS7w@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<CALNJ-vSojGWni=rpAjJqf6-sUWoL-_aLQwWTVoQQS2j2g34GKg@mail.gmail.com>
<CALNJ-vSmJbaoUm7nuyV65UdeYTee2UxqG49CZ7CjR_rEB5ZdJg@mail.gmail.com>
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
Attachments:
[application/gzip] 0001-Add-common-SQL-JSON-clauses-v52.patch.gz (8.1K, ../[email protected]/3-0001-Add-common-SQL-JSON-clauses-v52.patch.gz)
download
[application/gzip] 0002-SQL-JSON-constructors-v52.patch.gz (34.2K, ../[email protected]/4-0002-SQL-JSON-constructors-v52.patch.gz)
download
[application/gzip] 0003-IS-JSON-predicate-v52.patch.gz (12.0K, ../[email protected]/5-0003-IS-JSON-predicate-v52.patch.gz)
download
[application/gzip] 0004-SQL-JSON-query-functions-v52.patch.gz (39.8K, ../[email protected]/6-0004-SQL-JSON-query-functions-v52.patch.gz)
download
[application/gzip] 0005-SQL-JSON-functions-for-json-type-v52.patch.gz (12.6K, ../[email protected]/7-0005-SQL-JSON-functions-for-json-type-v52.patch.gz)
download
[application/gzip] 0006-GUC-sql_json-v52.patch.gz (5.0K, ../[email protected]/8-0006-GUC-sql_json-v52.patch.gz)
download
view thread (136+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: SQL/JSON: functions
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox