Thanks for the review, Fabrízio.
Attached is the updated patch, rebased and tested against master.
I integrated feedback from your first point and am no longer
HS_POLLUTE'ing the namespace for the new function.
With regards to your second point- I've been maintaining consistency
with the rest of the hstore module. Hstore's _size is internally
stored as a uint, but all uses of HS_COUNT across the feature end up
stored in a signed int. I could only find (grep) a few occurrences of
PG_RETURN_UINT32 across the entire codebase, and none in the hstore
module. If there's strong consensus for change, though, I'm happy to
do so.
Thanks,
Korbin Hoffman
On Mon, Jun 6, 2016 at 1:23 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
>
> On Fri, Jun 3, 2016 at 7:58 AM, Korbin Hoffman <k1@k1.io> wrote:
>>
>> Hi there-
>>
>> I've attached a small patch exposing HS_COUNT to the user as
>> "hstore_length(hstore)". Documentation, upgrade sql files, and a few
>> tests are also included.
>>
>> Almost every hstore function calls HS_COUNT, but I couldn't determine
>> if there was a reason this exposure didn't already exist.
>>
>> Without this function, I've been converting an hstore into an array
>> and then counting it - a more expensive operation (~30-40% slower than
>> SELECTing the hstore itself in a few of my tests).
>>
>> I will add this thread and patch to the next Commitfest.
>>
>
> Something goes wrong when applying against master:
>
> $ git apply ~/Downloads/hstore_length-v1.patch
> error: contrib/hstore/Makefile: already exists in working directory
> error: contrib/hstore/expected/hstore.out: already exists in working
> directory
> error: contrib/hstore/hstore--1.3.sql: already exists in working directory
> error: contrib/hstore/hstore.control: already exists in working directory
> error: contrib/hstore/hstore_op.c: already exists in working directory
> error: contrib/hstore/sql/hstore.sql: already exists in working directory
> error: doc/src/sgml/hstore.sgml: already exists in working directory
>
>
> Anyway I have some comments:
>
> 1) I don't see any reason to add this sort of thing if you're adding a new
> function
>
> + HSTORE_POLLUTE(hstore_length, length);
>
>
> 2) Shouldn't this declaration use 'uint32' instead of 'int' ??
>
> + int count = HS_COUNT(hs);
> +
> + PG_RETURN_INT32(count);
>
> maybe
>
> + uint32 count = HS_COUNT(hs);
> +
> + PG_RETURN_UINT32(count);
>
> Regards,
>
> --
> Fabrízio de Royes Mello
> Consultoria/Coaching PostgreSQL
>>> Timbira: http://www.timbira.com.br
>>> Blog: http://fabriziomello.github.io
>>> Linkedin: http://br.linkedin.com/in/fabriziomello
>>> Twitter: http://twitter.com/fabriziomello
>>> Github: http://github.com/fabriziomello