Re: [PATCH] Negative Transition Aggregate Functions (WIP)

On 22 January 2014 09:54, David Rowley <dgrowleyml@gmail.com> wrote:
> I've performed some more benchmarks on this patch tonight. The results and
> full recreation scripts are attached along with the patch it was tested
> against.
>

I noticed that the rate of changes to this patch has dropped off,
which I took as sign that it is ready for review, so I started doing
that.

My first thought was wow, this is a big patch!

I think it should probably be broken up. It might be overly ambitious
to try to get all of this committed during this commitfest, and in any
case, I suspect that the committer would probably choose to commit it
in stages. Perhaps something like:

Patch 1- Basic support for inverse transition functions, CREATE AGGREGATE
support and doc updates. This should include test cases to validate
that the underlying executor changes are correct, by defining custom
aggregates such as sum(int) and array_agg() using inverse transition
functions.

Patch 2- Add built-in inverse transition functions for count, sum(int), and friends.

Patch 3, 4...- Other related groups of built-in aggregates. By this point, it
should be a fairly mechanical process.

Splitting it up this way now should help to focus on getting patch 1
correct, without being distracted by all the other aggregates that may
or may not usefully be made to have inverse transition functions. I
think the value of the feature has been proved, and it is good to see
that it can be applied to so many aggregates, but let's not try to do
it all at once.


Regarding what would be in patch 1, I've only given it a cursory look,
but I did immediately notice a problem with the nodeWindowAgg.c
changes --- for aggregates with non-strict transition functions,
something equivalent to the not null counting code is needed to make
it work correctly with FILTER. For example, the following query gives
the wrong results with this patch:

SELECT array_agg(i) FILTER (WHERE i%3=0)      OVER (ROWS BETWEEN 1 PRECEDING AND CURRENT ROW) FROM
generate_series(1,10)g(i);
 

Perhaps it should just always count the non-skipped values added after
removal of filtered and null values. Then you might be able to
simplify things by getting rid of ignore_nulls from
WindowStatePerAggData and replacing notnullcount with a more general
valueCount to track the number of values actually added to transValue.

I haven't read through nodeWindowAgg.c in any detail yet (I think it
will take me a while to fully get my head round that code) but I think
it needs some more test cases to verify that the various corner cases
are covered.

Regards,
Dean