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

Re: Review: listagg aggregate

From: Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: "David E(dot) Wheeler" <david(at)kineticode(dot)com>, "pgsql-hackers(at)postgresql(dot)org Hackers" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Review: listagg aggregate
Date: 2010-01-28 02:47:07
Message-ID: 20100128114706.987A.52131E4D@oss.ntt.co.jp (view raw or flat)
Thread:
Lists: pgsql-hackers
Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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


Attachment: string_agg_20100128.patch
Description: application/octet-stream (10.9 KB)

In response to

Responses

pgsql-hackers by date

Next:From: David E. WheelerDate: 2010-01-28 03:28:31
Subject: Re: Review: listagg aggregate
Previous:From: Andrew DunstanDate: 2010-01-28 02:44:55
Subject: Re: make everything target

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