Re: array_accum aggregate

* Neil Conway (neilc(at)samurai(dot)com) wrote:
> On Wed, 2006-10-11 at 00:51 -0400, Stephen Frost wrote:
> > Here, the actual state type for any aggregate call is the array type
> > ! having the actual input type as elements. Note: array_accum() is now
> > ! a built-in aggregate which uses a much more efficient mechanism than
> > ! that which is provided by array_append, prior users of array_accum()
> > ! may be pleasantly suprised at the marked improvment for larger arrays.
> > </para>
>
> Not worth documenting, I think.

Ok. Either way is fine with me, honestly.

> > *** 15,20 ****
> > --- 15,22 ----
> > #include "utils/array.h"
> > #include "utils/builtins.h"
> > #include "utils/lsyscache.h"
> > + #include "utils/memutils.h"
> > + #include "nodes/execnodes.h"
>
> Include directives should be sorted roughly alphabetically, with the
> exception of postgres.h and system headers.

Ah, yeah, you know I noticed that when I added memutils but wasn't
thinking when I went back and added execnodes (I had been trying to
minimize the number of includes by adding ones I needed one at a time
and seeing if any more were necessary).

> > + /* Structure, used by aaccum_sfunc and aaccum_ffunc to
> > + * implement the array_accum() aggregate, for storing
> > + * pointers to the ArrayBuildState for the array we are
> > + * building and the MemoryContext in which it is being
> > + * built. Note that this structure is
> > + * considered an 'anyarray' externally, which is a
> > + * variable-length datatype, and therefore
> > + * must open with an int32 defining the length. */
>
> Gross.

Indeed. :/

> > + /* Make sure we are being called in an aggregate. */
> > + if (!fcinfo->context || !IsA(fcinfo->context, AggState))
> > + ereport(ERROR,
> > + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > + errmsg("Can not call aaccum_sfunc as a non-aggregate"),
> > + errhint("Use the array_accum aggregate")));
>
> Per the error message style guidelines, errmsg() should begin with a
> lowercase letter and errhint() should be a complete sentence (so it
> needs punctuation, for example).

Ah, ok, guess I should go read those.

> > + Datum
> > + aaccum_ffunc(PG_FUNCTION_ARGS)
> > + {
> > + aaccum_info *ainfo;
> > +
> > + /* Check if we are passed in a NULL */
> > + if (PG_ARGISNULL(0)) PG_RETURN_ARRAYTYPE_P(NULL);
>
> There is no guarantee why SQL NULL and PG_RETURN_XYZ(NULL) refer to the
> same thing -- use PG_RETURN_NULL() to return a SQL NULL value, or just
> make the function strict.

Huh, alright. I'll probably just change it to PG_RETURN_NULL().

Thanks!

Stephen