Re: Function array_agg(array) - Mailing list pgsql-hackers

From Ali Akbar
Subject Re: Function array_agg(array)
Date
Msg-id CACQjQLrMsvc=xDv7Rn4QB6exQVOTQ5xHmdFS7dXAD5NocEdbUw@mail.gmail.com
Whole thread Raw
In response to Re: Function array_agg(array)  (Pavel Stehule <pavel.stehule@gmail.com>)
Responses Re: Function array_agg(array)
List pgsql-hackers
Thanks for the review

2014-10-22 20:51 GMT+07:00 Pavel Stehule <pavel.stehule@gmail.com>:
I agree with your proposal. I have a few comments to design:

1. patch doesn't hold documentation and regress tests, please append it.
OK, i'll add the documentation and regression test 
 
2. this functionality (multidimensional aggregation) can be interesting more times, so maybe some interface like array builder should be preferred.
We already have accumArrayResult and makeArrayResult/makeMdArrayResult, haven't we?

Actually array_agg(anyarray) can be implemented by using accumArrayResult and makeMdArrayResult, but in the process we will deconstruct the array data and null bit-flags into ArrayBuildState->dvalues and dnulls. And we will reconstruct it through makeMdArrayResult. I think this way it's not efficient, so i decided to reimplement it and memcpy the array data and null flags as-is.

In other places, i think it's clearer if we just use accumArrayResult and makeArrayResult/makeMdArrayResult as API for building (multidimensional) arrays.
 
3. array_agg was consistent with array(subselect), so it should be fixed too

postgres=# select array_agg(a) from test;
       array_agg      
-----------------------
 {{1,2,3,4},{1,2,3,4}}
(1 row)

postgres=# select array(select a from test);
ERROR:  could not find array type for data type integer[]

I'm pretty new in postgresql development. Can you point out where is array(subselect) implemented?

 
4. why you use a magic constant (64) there?

+         astate->abytes = 64 * (ndatabytes == 0 ? 1 : ndatabytes);
+         astate->aitems = 64 * nitems;

+             astate->nullbitmap = (bits8 *)
+                 repalloc(astate->nullbitmap, (astate->aitems + 7) / 8);

just follow the arbitrary size choosen in accumArrayResult (arrayfuncs.c):
astate->alen = 64; /* arbitrary starting array size */

it can be any number not too small and too big. Too small, and we will realloc shortly. Too big, we will end up wasting memory.

Regards,
--
Ali Akbar

pgsql-hackers by date:

Previous
From: Antonin Houska
Date:
Subject: Re: from_collapse_limit considerations
Next
From: Merlin Moncure
Date:
Subject: Re: Support UPDATE table SET(*)=...