Skip site navigation (1) Skip section navigation (2)

Re: Review: listagg aggregate

From: "David E(dot) Wheeler" <david(at)kineticode(dot)com>
To: Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Cc: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org Hackers" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Review: listagg aggregate
Date: 2010-01-28 03:28:31
Message-ID: 950D0CD5-8FE4-4984-A336-A7B9DD62116C@kineticode.com (view raw or flat)
Thread:
Lists: pgsql-hackers
On Jan 27, 2010, at 6:47 PM, Takahiro Itagaki wrote:

> * I think we cannot cache the delimiter at the first call.
>  For example,
>    SELECT string_agg(elem, delim)
>      FROM (VALUES('A', ','), ('B', '+'), ('C', '*')) t(elem, delim);
>  should return 'A+B*C' rather than 'A,B,C'.

Ooh, nice.

> * Can we use StringInfo directly as the aggregate context instead of
>  StringAggState? For the first reason, we need to drop 'delimiter' field
>  from struct StringAggState. Now it has only StringInfo field.

Makes sense.

> * We'd better avoiding to call text_to_cstring() for delimitors and elements
>  for performance reason. We can use appendBinaryStringInfo() here.
> 
> My proposal patch attached.
> 
> Also, I've not changed it yet, but it might be considerable:
> 
> * Do we need better names for string_agg1_transfn and string_agg2_transfn?
>  They are almost "internal names", but we could have more 
>  like string_agg_with_sep_transfn.

Yes please.

> Comments?

Patch looks great, thank you!

David



In response to

Responses

pgsql-hackers by date

Next:From: Greg SmithDate: 2010-01-28 04:13:05
Subject: Re: CommitFest status summary 2010-01-27
Previous:From: Takahiro ItagakiDate: 2010-01-28 02:47:07
Subject: Re: Review: listagg aggregate

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group