Re: Review Report: propose to include 3 new functions into intarray and intagg - Mailing list pgsql-hackers

<div dir="ltr">OK, thank you for your review.<br /><br />I'll correct everything and send a patch in a couple of days.
Areyou completely sure that this patch will be included? If not, seems the work of the patch standartization has much
lowerpriority, and I will not hurry so much.<br /><br />But, what about intarray patch? Does somebody plan to review
it?I'd prefer to include it too. If you approve, I'll correct the code style in intarray contrib patch too.<br /><br
/><br/><br /><div class="gmail_quote">On Sat, Sep 6, 2008 at 3:34 PM, Markus Wanner <span
dir="ltr"><markus@bluegap.ch></span>wrote:<br /><blockquote class="gmail_quote" style="border-left: 1px solid
rgb(204,204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> Hi,<br /><br /> this is my first "official" review.
I'vetried to follow the "Review a<br /> patch" guidelines from the wiki - thanks Simon, that was pretty helpful.<br
/><br/> This review covers only the intagg additions.<br /><br /> Dmitry Koterov wrote:<br /><blockquote
class="gmail_quote"style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hereare these functions with detailed documentation:<br /><a href="http://en.dklab.ru/lib/dklab_postgresql_patch/"
target="_blank">http://en.dklab.ru/lib/dklab_postgresql_patch/</a><br/></blockquote><br /> Submission review: we
generallyprefer having patches archived on our<br /> mailing lists, so please just send future patches or revisions of
this<br/> patch to our lists (I prefer -hackers, but probably -patches is still<br /> the official one). The
documentationshould go into a README file of the<br /> contrib module or some such. Tests are missing, but that's the
casefor<br /> the intagg contrib module anyway. The patch applies and is in context<br /> diff format, good. It adds an
unrelatednewline, which should generally be avoided.<br /><br /> Usability review: functional additions look good and
usable,certainly<br /> within a contrib module. I don't think it's compliant to any spec, but<br /> that's not required
forcontrib, IMO.<br /><br /> Functional test: works as advertised on the accompanying website. I've<br /> tested the
int_array_append_aggregatesomewhat, see [1].<br /><br /> Performance testing: I've tested with 10 mio rows of arrays of
size1<br /> and compared against core's int_array_aggregate function, see [1] again.<br /> In this simple test it took
roughly50% longer, which seems okay. Memory consumption looks sane as well.<br /><br /> Coding review: style seems fine
forcontrib, though lines longer than 80 cols should be broken up. Comments in the code are sparse , some even
counter-productive(marking something as "additional things" certainly doesn't help).<br /><br /> Code and architecture
review:the new int_array_append_aggregate() functions itself seems fine to me.<br /><br /> Summary: My general feeling
is,that this patch should be applied after minor code style corrections. As a longer term goal I think intagg should be
integratedinto  core, since it's very basic functionality. TODO entries for things like an array_accum() aggregate
alreadyexist. Adding this patch to contrib now might be a step into the right direction.<br /><br /> Dmitry, can you
pleaseapply these small corrections and re-submit the patch?<br /><br /> Regards<br /><br /> Markus Wanner<br /><br />
P.S.:I dislike the intagg's use of PGARRAY, but that's nothing to do with this patch. Shouldn't this better use a real
compositetype as the aggregate's state type? I'd propose to clean up the intagg contrib module and prepare it for
inclusioninto core.<br /><br /><br /> [1]: functional and performance testing session:<br /><br /> On a database with
(patched)intagg and intarr contrib modules:<br /><br /> markus=# CREATE TABLE test (id INT NOT NULL, arr INT[] NOT
NULL);<br/> CREATE TABLE<br /> markus=# INSERT INTO test VALUES (1, ARRAY[1,2,3]), (2, ARRAY[4,5]), (3,<br />
ARRAY[3,2,1]);<br/> INSERT 0 3<br /> markus=# SELECT * FROM test;<br />  id |   arr<br /> ----+---------<br />  1 |
{1,2,3}<br/>  2 | {4,5}<br />  3 | {3,2,1}<br /> (3 rows)<br /><br /> markus=# SELECT int_array_append_aggregate(arr)
FROMtest;<br />  int_array_append_aggregate<br /> ----------------------------<br />  {1,2,3,4,5,3,2,1}<br /> (1
row)<br/><br /> markus=# SELECT * FROM test;<br />  id |   arr<br /> ----+---------<br />  1 | {1,2,3}<br />  2 |
{4,5}<br/>  3 | {3,2,1}<br /> (3 rows)<br /><br /> markus=# SELECT int_array_aggregate(id) AS ids,<br />
int_array_append_aggregate(arr)FROM test GROUP BY (id / 2);<br />  ids  | int_array_append_aggregate<br />
-------+----------------------------<br/>  {1}   | {1,2,3}<br />  {2,3} | {4,5,3,2,1}<br /> (2 rows)<br /><br />
markus=#SELECT int_array_aggregate(id) AS ids,<br /> int_array_append_aggregate(arr) FROM test GROUP BY (id % 2);<br />
 ids | int_array_append_aggregate<br /> -------+----------------------------<br />  {2}   | {4,5}<br />  {1,3} |
{1,2,3,3,2,1}<br/> (2 rows)<br /><br /> markus=# INSERT INTO test VALUES (4, NULL);<br /> INSERT 0 1<br /><br />
markus=#SELECT int_array_append_aggregate(arr) FROM test;<br />  int_array_append_aggregate<br />
----------------------------<br/>  {1,2,3,4,5,3,2,1}<br /> (1 row)<br /><br /> markus=# SELECT id,
int_array_append_aggregate(arr)FROM test GROUP BY id;<br />  id | int_array_append_aggregate<br />
----+----------------------------<br/>  4 | {}<br />  2 | {4,5}<br />  3 | {3,2,1}<br />  1 | {1,2,3}<br /> (4 rows)<br
/><br/> -- switching to performance testing<br /><br /> markus=# \timing<br /> Timing is on.<br /><br /> markus=#
DELETEFROM test;<br /> DELETE 4<br /> Time: 9.037 ms<br /><br /> markus=# INSERT INTO test SELECT generate_series(1,
10000000),<br/> array[round(random() * 100)]::int[];<br /> INSERT 0 10000000<br /> Time: 53321.186 ms<br /><br />
markus=#SELECT icount(int_array_aggregate(id)) AS count FROM test;<br />  count<br /> ----------<br />  10000000<br />
(1row)<br /><br /> Time: 2493.184 ms<br /><br /> markus=# SELECT icount(int_array_append_aggregate(arr)) AS count FROM
test;<br/>  count<br /> ----------<br />  10000000<br /> (1 row)<br /><br /> Time: 4152.478 ms<br /><br /><br
/></blockquote></div><br/></div>