Re: [PATCH] Generic type subscripting

On 28.03.2017 19:31, Dmitry Dolgov wrote:
> On 28 March 2017 at 12:08, Dmitry Dolgov <9erthalion6@gmail.com
> <mailto:9erthalion6@gmail.com>> wrote:
>>
>> Wow, I didn't notice that, sorry - will fix it shortly.
>
> So, here is the corrected version of the patch.

I have some picky comments.

I'm not sure that "typsbsparse" is better than "typsubscripting" or 
"typsubparse". Maybe "typsubsparse"?

>       <row>
> +      <entry><structfield>typsubscripting</structfield></entry>
> +      <entry><type>regproc</type></entry>

Here you didn't fix "typsubscripting" to new name.

> +  <title>JSON subscripting</title>
> +  <para>
> +   JSONB data type support array-style subscripting expressions to extract or update particular element. An example
ofsubscripting syntax:
 

Should be "JSONB data type supports".

> +  to handle subscripting expressions. It should contains logic for verification
> +  and decide which function must be used for evaluation of this expression.
> +  For instance:

Should be "It should contain".

> + <sect2 id="json-subscripting">
> +  <title>JSON subscripting</title>
> +  <para>
> +   JSONB data type support array-style subscripting expressions to extract or update particular element. An example
ofsubscripting syntax:
 

You have implemented jsonb subscripting. The documentation should be 
fixed to:

+ <sect2 id="jsonb-subscripting">
+  <title><type>jsonb</> Subscripting</title>
+  <para>
+   <type>jsonb</> data type support array-style subscripting 
expressions to extract or update particular element. An example of 
subscripting syntax:

I think IsOneOf() macros should be removed. Since it is not used anywhere.

> +        Assert(subexpr != NULL);
> +
> +        if (subexpr == NULL)
> +            ereport(ERROR,
> +                    (errcode(ERRCODE_DATATYPE_MISMATCH),
> +                     errmsg("jsonb subscript does not support slices"),
> +                     parser_errposition(pstate, exprLocation(
> +                        ((Node *) lfirst(sbsref->refupperindexpr->head))))));

Here one of the conditions is excess. Better to delete assert condition 
I think.

I've tried tests from message [1]. They looks good. Performance looks 
similar for subscripting without patch and with patch.

I wanted to implement subscripting for ltree or hstore extensions. 
Subscripting for ltree looks more interesting. Especially with slicing. 
But I haven't done it yet. I hope that I will implement it tomorrow.

1. 
https://www.postgresql.org/message-id/CAKNkYnz_WWkzzxyFx934N%3DEp47CAFju-Rk-sGeZo0ui8QdrGmw%40mail.gmail.com

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company