Review: listagg aggregate
in [PgSql]
|
From: "David E. Wheeler" on 27 Jan 2010 19:45 On Jan 27, 2010, at 7:58 AM, Pavel Stehule wrote: > with actualised oids Thanks. Looks good, modulo my preference for concat_agg(). I'll mark it ready for committer. Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Takahiro Itagaki on 27 Jan 2010 21:47 Pavel Stehule <pavel.stehule(a)gmail.com> wrote: > with actualised oids I'm checking the patch for commit, and have a couple of comments. * 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'. * 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. * 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. Comments? Regards, --- Takahiro Itagaki NTT Open Source Software Center
From: "David E. Wheeler" on 27 Jan 2010 22:28 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 28 Jan 2010 03:37 2010/1/28 Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp>: > > Pavel Stehule <pavel.stehule(a)gmail.com> wrote: > >> with actualised oids > > I'm checking the patch for commit, and have a couple of comments. > > * 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'. no I dislike it. This using is nonsense. Regards Pavel > > * 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. > > * 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. > > Comments? > > Regards, > --- > Takahiro Itagaki > NTT Open Source Software Center > > -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Pavel Stehule on 28 Jan 2010 03:38
2010/1/28 David E. Wheeler <david(a)kineticode.com>: > 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. no, has not. Pavel > >> * 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 > > > -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |